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

feat: Allow to reorder options of "checkbox" "radio" and "dropdown" question types in frontend #2092

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

susnux
Copy link
Collaborator

@susnux susnux commented Apr 22, 2024

This adds support to reorder options instead of having to remove them.
It works but I had to modify a lot of places to keep smooth behavior of the list ordering.

Implementation things:

  • I am not sure about the API endpoint, it does not match our current naming style
  • Should we expose the "order" property or just keep it internally and only return options sorted?

@nextcloud/designers question:

  • There is not much space for a "drag" handle + a11y button so I just used buttons, I do not think it looks super awesome, but it works. Do you have an idea how to improve? Especially because there is already the drag handle for the question itself.

Screenshots

Shuffle options enabled = no sorting No shuffle = you can sort
Screenshot 2024-04-22 at 12-31-10 fff9 - Forms - Nextcloud Screenshot 2024-04-22 at 12-31-51 fff9 - Forms - Nextcloud

@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews labels Apr 22, 2024
@susnux susnux added this to the 4.3 milestone Apr 22, 2024
@susnux susnux force-pushed the feat/reorder-options branch from 2faf119 to ee71d9d Compare April 22, 2024 11:07
@szaimen
Copy link

szaimen commented Apr 22, 2024

  • There is not much space for a "drag" handle + a11y button so I just used buttons, I do not think it looks super awesome, but it works. Do you have an idea how to improve? Especially because there is already the drag handle for the question itself.

I think it looks fine but had one suggestion :)

@susnux susnux force-pushed the feat/reorder-options branch from ee71d9d to ef615de Compare April 22, 2024 11:25
Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

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

Some things I found by looking at the code, did not test it by now.

@susnux susnux force-pushed the feat/reorder-options branch from ef615de to 18901ad Compare April 22, 2024 11:59
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

There is not much space for a "drag" handle + a11y button so I just used buttons, I do not think it looks super awesome, but it works. Do you have an idea how to improve? Especially because there is already the drag handle for the question itself.

@susnux what about using a drag handle that on click/enter shows a menu saying "Move up" and "Move down"? Then it would be 1 icon less, and both cases would be covered.

Questions:

  • Should the drag handle be on left or right – I would tend towards left
  • Which icon to use. I’d say the simple "drag" – or we use that for the whole question, and the "drag-horizontal-variant" for the individual replies?

@Chartman123
Copy link
Collaborator

drag handle that on click/enter shows a menu saying "Move up" and "Move down"

@jancborchardt This would on the other hand introduce one more click for every move operation...

  • Should the drag handle be on left or right – I would tend towards left

having the handle on the left would introduce one more difference between edit and view mode, but we already have some differences, so I'd be fine with that. Whatever looks better as a whole.

@susnux
Copy link
Collaborator Author

susnux commented Apr 23, 2024

The problem with a drag handle on the left is the conflict with the drag handle for the question itself.
We would then have two drag handles next to each other. Not sure about that.

@susnux susnux force-pushed the feat/reorder-options branch 2 times, most recently from 69939ee to fecb454 Compare April 23, 2024 10:05
@Chartman123
Copy link
Collaborator

@susnux @jancborchardt What about having the arrow icons as an overlay at the end of the input fields that only shows up when the field has the focus / hover?

@susnux should I add your backend code to the API v3 PR or would you like to keep it in here and adjust it after v3 merge?

@susnux
Copy link
Collaborator Author

susnux commented Aug 11, 2024

adjust it after v3 merge?

will adjust afterwards :)

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

What about having the arrow icons as an overlay at the end of the input fields that only shows up when the field has the focus / hover?

@Chartman123 sounds good! Then it’s not that much visual noise, and the fields are nicely aligned in width. :)

@Chartman123 Chartman123 modified the milestones: 4.3, 5.0 Sep 23, 2024
@Chartman123 Chartman123 changed the title feat: Allow to reorder options of "checkbox" "radio" and "dropdown" question types feat: Allow to reorder options of "checkbox" "radio" and "dropdown" question types in frontend Sep 23, 2024
@Chartman123 Chartman123 force-pushed the feat/reorder-options branch 5 times, most recently from b746bea to d087b97 Compare January 25, 2025 16:26
@Chartman123 Chartman123 force-pushed the feat/reorder-options branch 2 times, most recently from c688bff to 83826ce Compare February 16, 2025 15:48
@Chartman123
Copy link
Collaborator

This is now rebased to our current main. Design-wise there's still some room for improvement as I'm really no expert in CSS stuff. But I already made the arrows overlay the input fields.

@jancborchardt what do you think?

grafik

@marcoambrosini
Copy link
Member

marcoambrosini commented Feb 17, 2025

@Chartman123 @jancborchardt is out on vacation and I'm replacing him.
The arrows look good! Please make sure that their respective buttons are of --clickable-area size and that the icons are centered in them. Since these are tertiary buttons we don't need any extra margins.
One question: shouldn't the arrows show only when the input field is focused?

Other than that, for the left drag handle I would suggest this icon:
https://fonts.google.com/icons?selected=Material+Symbols+Outlined:drag_indicator:FILL@0;wght@400;GRAD@0;opsz@24&icon.query=drag&icon.size=24&icon.color=%23e8eaed
Screenshot 2025-02-17 at 14 25 34

This drag button looks like it has too much right margin, I'd reduce that to --default-grid-baseline

@Chartman123
Copy link
Collaborator

One question: shouldn't the arrows show only when the input field is focused?

@marcoambrosini yes, but this might perhaps also be an a11y issue? and we can also deliver it in one of the next minor/patch releases :)

The arrows look good! Please make sure that their respective buttons are of --clickable-area size and that the icons are centered in them. Since these are tertiary buttons we don't need any extra margins.

Is the size not managed directly by using the NCButton?

@marcoambrosini
Copy link
Member

@Chartman123 personally if we had to start from scatch, I would add two action buttons triggered by a click of the drag button. Adding the two options there. So that:

  • click and drag allows direct reordering
  • keyboard navigators can hit the drag button and open an actions menu with the move up and move down options

@Chartman123
Copy link
Collaborator

@marcoambrosini so instead of the arrows on the right a drag handle (also on the right) and the up/down actions behind the action menu?

@marcoambrosini
Copy link
Member

Yes exactly

* @param {number} index Option that should move up
*/
onOptionMoveUp(index: number) {
if (index > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need for this if, because the button that calls this logic will be disable when index = 0

* @param {number} index Option that should move down
*/
onOptionMoveDown(index: number) {
if (index === this.sortedOptions.length - 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need for this if, because the button that calls this logic will be disable when index = this.sortedOptions.length - 1

@AIlkiv
Copy link
Collaborator

AIlkiv commented Feb 20, 2025

I don't think you should overlap the input borders on hover. I tested on version 30.

image

@Chartman123
Copy link
Collaborator

I don't think you should overlap the input borders on hover. I tested on version 30.

Yes this has to be fixed, but not a blocker as the buttons will also be replaced in #2579 by a drag handle

@Chartman123 Chartman123 force-pushed the feat/reorder-options branch 2 times, most recently from 866cd72 to acfe0a3 Compare February 20, 2025 15:56
@Chartman123
Copy link
Collaborator

Chartman123 commented Feb 20, 2025

@max-nextcloud any idea why the e2e tests are failing without my changes? I mean we're still using aria-labels, even if they are now coming from a computed prop: acfe0a3

Edit: I found the error... We changed the text for existing options and the aria-label had a wrong condition

susnux and others added 6 commits February 20, 2025 18:06
…ype options

Signed-off-by: Ferdinand Thiessen <[email protected]>
Signed-off-by: Christian Hartmann <[email protected]>
Signed-off-by: Christian Hartmann <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: Christian Hartmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple choice or checkbox answers to be reordered
6 participants