ColorPicker: Only display detailed inputs #41222
Merged
Conversation
This file contains 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
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: -199 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work @aaronrobertshaw, the code change looks good to me, and having an explicit Copy button feels like a better use of space to me (and it also feels nicer being able to immediately interact with the Hex input field).
I noticed that each of the non-Hex inputs cause there to be horizontal and vertical scrollbars in my environment. I was wondering if the AuxiliaryColorArefactWrapper is slightly too big for the popover, and if it might need to be constrained somehow.
I see this is the same for those components on trunk, though, so also fine if you think we should address that separately (I think it felt a bit more obvious to me now that the dropdown to change color types is more prominent). What do you think?
|
Thanks for reviewing @andrewserong 👍
Another reason for this was non-hex color models can't simply be copied from an individual input field. It made sense to me that the UI also be consistent between the different color models.
I noticed this as well when testing the recent change of Popovers to the floatingUI library. Given it was an existing problem on trunk, my plan was to address it separately. |
That sounds reasonable to me. I've tested in Storybook + and in the post and site editors and this is otherwise looking good as far as I can tell (code looks good 👍). I couldn't quite work out why the Heading e2e test is failing, since it looks like it should be passing after your code change 🤔 (and stepping through those steps manually in the editor appears to work well) Happy to do more testing / review again tomorrow! |
aaronrobertshaw
force-pushed
the
update/color-picker-input-display
branch
2 times, most recently
from
d1da718
to
58d4918
Compare
aaronrobertshaw
force-pushed
the
update/color-picker-input-display
branch
from
58d4918
to
2542ad6
Compare
andrewserong
approved these changes
May 26, 2022
There was a problem hiding this comment.
Thanks for fixing up the e2e tests! This is still testing nicely for me after the rebase.
Unrelated to this PR, I've noticed some more slightly unusual behaviour with the Popover, on shorter screens when we open the popover closer to the bottom of the screen. When we switch between different color types, instead of the Popover moving up, we get a longer vertical scrollbar as in the screengrab below:
I believe this is to do with the size middleware we're using in the Popover component's middleware (currently it sets a maximum height, which is based on the available space between where the popover opened and the edge of the viewport). I think, along with the horizontal scrollbar, it could be a good thing for us to look at trying to tweak in follow-ups — potentially within the Popover logic. I ran into some related issues while working on #41361 (which looks at removing the maxWidth setting), so just thought I'd mention it in case you've run into similar issues while testing this PR.
All that said, this PR looks good to me, and like I mentioned earlier, I think it's a much nicer way for it to work, with an immediately usable Hex field and a nice big Copy button 👍
LGTM! 🎉
|
Thanks for reviewing this again 🙇
Sounds like a plan 👍 I'll get this merged to resolve #37981. |
I've opened up a (non-urgent) fix for the horizontal scrollbar in #41646 |
19 tasks
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.
Related:
What?
Removes the requirement to toggle display of detailed input fields for the ColorPicker's different color models.
This PR only addresses the UI change not updating pasted hex values. That will be addressed separately.
Why?
To ease the editing of color values by reducing assumed knowledge and the number of clicks required to make a change via the inputs.
Further reasoning and discussion can be found in: #37981
How?
ColorCopyButtoncomponent internal to theColorPicker.ColorDisplaycomponent.Testing Instructions
npm run test-unit packages/components/src/color-picker/testScreenshots or screencast
Screen.Recording.2022-05-23.at.3.06.17.pm.mp4
Screen.Recording.2022-05-23.at.3.06.48.pm.mp4