Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/ Edge Browser: support textbox input when the edit invitation is not an enum #2095

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

xkopenreview
Copy link
Collaborator

this pr should allow a textbox in edge browser to add/edit edge

example for weight of custom max papers:

"weight": {
    "param": {
      "input": "text",
      "maximum": 100,
      "minimum": 0
    }
  }

should show a textbox for custom max papers edge

return column
})
return { columns: resultColumns }
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data loading is updated to avoid state update conflict

reloadColumnEntities update was overwritting state update from updateChildColumns

@celestemartinez
Copy link
Member

How does a default value work here? Because when I set a default value and I try to assign a reviewer that already has that number of assignments, the edge browser won't let me make the assignment, but I don't see this default value anywhere and it's not clear why I can't make the assignment.

Also, it is not very straightforward how to change this value, and changing it is not very smooth. Not sure if this will be a good experience for PCs.

@xkopenreview
Copy link
Collaborator Author

xkopenreview commented Sep 17, 2024

please elaborate what makes you think it's not clear/smooth

  • it's slow when there are a lot of edges to loads. the edge updates after the column and edges are reloaded so the page may freeze for a moment. it's worse when the custom load is two digit because it update to first the 1 digit that user input then update again to the 2 digits.

@xkopenreview xkopenreview marked this pull request as draft September 17, 2024 21:07
@xkopenreview xkopenreview marked this pull request as ready for review September 18, 2024 17:38
Copy link
Member

@enrubio enrubio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things:

I think the trash icon should look the same as the Assignment one:
Screenshot 2024-09-26 at 4 31 05 PM

If you put invalid input, the spinner goes on forever with no way of editing your previous input. I think the expected behavior is to immediately edit and fix the issue. Otherwise you have to click another paper and click back, or trash the edge (if it exists):
spinner

I think we should also have some way of telling the PCs what the expected input is now that it's a textbox.

Copy link
Member

@enrubio enrubio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issues I mentioned have been fixed and I think it looks good. I was worried that the expected values/range wasn't as clear as it was with the dropdown, but I checked again and I think the validation errors are clear enough.

Example with and without a default (of 6):

cmp-text

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edge Browser: support textbox input when the edit invitation is not an enum
3 participants