-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ColorPicker: Remove horizontal scrollbar when using HSL or RGB color input types #41646
Conversation
b9e4190
to
721aa7e
Compare
Lena and Marco — I've added you both as reviewers, but there's no urgency with this one at all as I'll be finishing up for the week shortly and am AFK on Monday 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested well for me.
Code changes LGTM 🎉 I'll ping @jameskoster in case he has the time to take a look from the design perspective too! |
It's mostly working well, but I do still see a horizontal scrollbar appearing intermittently (with scrollbars set to always display in macOS sys prefs): scroll.mp4Almost certainly unrelated, but another strange issue – when you drag the range to its minimum, the adjacent text input displays the maximum value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I believe ColorPalette
is the component you're looking for in the Storybook for testing — it's the one that has ColorPicker
wrapped in a popover ✨
It's mostly working well, but I do still see a horizontal scrollbar appearing intermittently
Yes, it looks like the main color picker dot (.react-colorful__pointer
) is overflowing the container. I looked into this and noticed this line here:
gutenberg/packages/components/src/color-palette/style.scss
Lines 29 to 30 in 2fa50a6
.components-dropdown__content.components-color-palette__custom-color-dropdown-content .components-popover__content { | |
overflow: visible; |
I think this is what we want (vis-à-vis the dot, and maybe even the sliders?), and it's certainly the intent of the code. However, there's some JS somewhere that's adding an inline style that's overriding the overflow
:
So if you overflow: visible !important
, it's fixed. I have a feeling this is a regression (cf. the screenshot in #36963). It would be nice to get to the root of this, but if all else fails, maybe we resort to !important
😅
hideLabelFromVision | ||
__unstableStateReducer={ stateReducer } | ||
/> | ||
<Spacer as={ HStack } spacing={ 4 }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hack feels weird to me, in that hex-input doesn't need to be HStack
ed nor have spacing
— this Spacer
is only being used for the default margin-bottom that comes with it.
This might have hidden complexity that I'm missing so it's not a blocker for this PR, but I feel like the CSS structure should be refactored to be more like this:
This way we can use flex gap instead of the Spacer's margin-bottom, and don't have to wrestle with the final item's margin-bottom bleeding out and messing with our outermost padding.
721aa7e
to
a0b61e0
Compare
Thanks for the reviews and feedback @mirka and @jameskoster, apologies for the delay — I switched over to some other tasks this week, but have just given this a rebase and tweaked some things a little further. I've done the following:
Here's how it's looking now: Please let know if you think there's different / better trade-offs to be made, and I can follow up next week. Thanks! 🙇
Yes, that seems a bit weird, not too sure what's going on there. Happy to take a look at that separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me and the scrollbar issue seems to have been fixed:
scroll.mp4
Another tangential observation, it's a bit strange how the palette popover has different styling to the custom color one. I guess I'll open an issue for that one :)
Thanks for the follow-up review and for opening up the borders inconsistency in: #41787 👍 I'll merge this one in now, and we can look at follow-ups separately 🙂 |
What?
Raised in the review for #41222 (review):
In the ColorPicker component, ensure that the range controls in their 100% position don't cause the component to overflow and create a horizontal scrollbar when used in a popover.
Also, ensure that the bottom margin / padding of the input fields is consistent between each color input type.
Why?
It appears that the
RangeControl
component's slider handle (intentionally) overflows the width of the control so that the handle is more visible — the drawback is that this can cause this particular Popover to overflow. To fix this, we can give the component a little more breathing room. It seemed safer to fix it here, than to update theRangeControl
directly, where it could adversely affect other instances of that component.How?
Testing Instructions
To more clearly see when there's a horizontal scrollbar, if you're running macOS, go to System Preferences > General > Show scroll bars, and set it to Always.
(Note: in Storybook, the
ColorPicker
component is not rendered in a Popover, so it's best to test this component change in the editor)Screenshots or screencast