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 dropdown keyboard movement bug #1630

Merged

Conversation

elbertronnie
Copy link
Collaborator

Fixes a bug where the keyboard movements for Up and Down arrow in a Dropdown list do not behave properly.

This bug is part of code-todo-list channel in Discord: https://discord.com/channels/731730685944922173/881073965047636018/1111934349663805450

This bug happened because in many instances value parameter was not provided in MenuListEntry.

@Keavon
Copy link
Member

Keavon commented Feb 27, 2024

!build

Copy link

📦 Build Complete for ac0766b
https://3e9afbc1.graphite.pages.dev

@elbertronnie
Copy link
Collaborator Author

elbertronnie commented Feb 27, 2024

This is mostly a temporary fix. A much more permanent fix would be to make value as the constructor instead of label. This way there will be less chances of forgetting it. Should I do this?

And should I do a similar thing with RadioEntryData if it is ever decided that Radiobuttons will also be controlled by keyboard in the future?

@0HyperCube
Copy link
Member

Thanks for this fix.
I think making the value the constructor would be a good approach, possibly alongside the label. I can't see any use-cases where the value should be different from the label so perhaps we could merge them for the menu list entry struct?

For the RadioEntryData I think it is probably advantageous to have the value as a constructor argument as well as you suggested.

@elbertronnie
Copy link
Collaborator Author

elbertronnie commented Feb 27, 2024

I can't see any use-cases where the value should be different from the label so perhaps we could merge them for the menu list entry struct?

There may be cases in future where icon is used with no label. Merging them would break that. Radiobutton has this kind of use case already.

@Keavon
Copy link
Member

Keavon commented Feb 27, 2024

Likewise, I believe localizations would result in a different value and label.

@Keavon
Copy link
Member

Keavon commented Feb 28, 2024

This is mostly a temporary fix. A much more permanent fix would be to make value as the constructor instead of label. This way there will be less chances of forgetting it. Should I do this?

And should I do a similar thing with RadioEntryData if it is ever decided that Radiobuttons will also be controlled by keyboard in the future?

Yes please, to both!

@Keavon
Copy link
Member

Keavon commented Feb 29, 2024

I'll temporarily set this to a draft again while awaiting that change you suggested @elbertronnie. Feel free to mark it as ready for review again when you're ready, and also please ping me so I notice.

@Keavon Keavon marked this pull request as draft February 29, 2024 05:54
@elbertronnie elbertronnie marked this pull request as ready for review March 2, 2024 14:41
@elbertronnie
Copy link
Collaborator Author

@Keavon I have made value a constructor in both MenuListEntry and RadioEntryData. I have also updated all definitions of value to be equal to what Debug trait prints where possible. In other cases it has been hard-coded with a lowercase string.

@Keavon Keavon force-pushed the fix-dropdown-keyboard-movement branch from d7d7209 to 9a901c2 Compare March 3, 2024 00:46
Copy link
Member

@Keavon Keavon left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for this!

@Keavon Keavon merged commit 9479abe into GraphiteEditor:master Mar 3, 2024
2 checks passed
@Keavon
Copy link
Member

Keavon commented Mar 5, 2024

@elbertronnie I found a regression with the blend mode dropdown at the top of the Layers panel (next to the Opacity slider), since it seems to lack a label:

image
(shows the debug value for the first iteration of the Svelte template loop for the first entry)

Would you mind submitting a quick fix? (And if finding this makes you notice any other potentially forgotten lines of code, please double check those also if applicable.) Thanks!

Keavon pushed a commit that referenced this pull request Mar 5, 2024
@elbertronnie elbertronnie deleted the fix-dropdown-keyboard-movement branch March 15, 2024 17:43
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.

None yet

3 participants