Components: ColorPicker: replace global shortcut event handlers with local ones #34508
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +16 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
remove/color-picker-global-shortcuts
branch
from
3a0981c to
d245423
Compare
Member
jorgefilipecosta
left a comment
jorgefilipecosta
left a comment
There was a problem hiding this comment.
Hi @ellatrix,
I noticed a regression previously I could move the color circle using arrow keys and now it does not seems to work:
We have a redesign of the color picker in progress and the roadmap involves basically removing all the current component code #34284 and using a third-party component. So if you prefer we can wait a little bit for the component update.
Member
Author
|
@jorgefilipecosta It seems to work fine if I move focus to the slider with TAB, but clicking on the slider does not move focus to the slider for some reason. |
Member
Author
Why are we replacing it with a third party component? What's wrong with the current one? |
jorgefilipecosta
approved these changes
Sep 3, 2021
Member
jorgefilipecosta
left a comment
jorgefilipecosta
left a comment
There was a problem hiding this comment.
@jorgefilipecosta It seems to work fine if I move focus to the slider with TAB, but clicking on the slider does not move focus to the slider for some reason.
Yes I retested and it seems to move it arrows so I guess we can merge 👍
Member
Good question. I'm not inside of the reasons; I know that a redesign is planned, maybe using the third-party component makes the new design simpler to implement? |
Contributor
|
Thank you for the ping, @jorgefilipecosta ! We are not replacing the current ColorPicker with a third-party component; we are replacing it with a new, first-party version that aligns more closely with the design and the needs that were mostly outlined in the context of the Global Styles sidebar (see #27331 for more context) The new version of the Hope this clarifies your doubts! (cc'ing @mtias in case he wanted to add on top of my explanation) I wonder if, since the current version of the ColorPicker should soon be replaced by the new one and then deleted, we should make any changes to it. |
getsource
added
[Package] Components
ellatrix
force-pushed
the
remove/color-picker-global-shortcuts
branch
from
d245423 to
56093f0
Compare
Member
Author
|
Let's merge this. I plan to deprecate the KeyboardShortcuts component. Even if this component will be removed eventually, I assume at some point the new color picker component will also need keyboard shortcuts, so this will help with showing how to do it. |
Contributor
|
Makes sense — thank you for the explanation, @ellatrix! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Remove whitespace to review more easily.
Related: #34498, #34500, #34503, #34505.
KeyboardShortcuts adds event handles globally, while these handlers could simply be added locally instead.
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).