-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 color picker saturation bug #23272
Conversation
Size Change: +23 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
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 is great, thank you! It is relieving to see that there is a CSS-only fix to this.
I'm not really able to reproduce the issue as it was back in january, but I'll keep looking.
It looks like the issue changed slightly. That is it is no longer triggered by the right-edge of the picker but the bototm-edge:
This PR does fix this, and also makes the picker look and behave much better in general with how it is able to overflow.
I also noticed this odd issue with the popover location just now (however, also on master it doesn't seem to be a result of this change):
I haven't noticed this before. And I have used the color picker in the last week while helping develop some of the experimental block style support flags. Im not sure whats going here, but I figured it would be worth pointing out.
@@ -27,7 +27,7 @@ | |||
|
|||
.components-color-picker { | |||
width: 100%; | |||
overflow: hidden; | |||
overflow: visible; |
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.
Im not finding this change necessary to fix the bug. However, the change to visible
in .components-color-picker__saturation-color
does seem to be what does it!
Did you have any other reasons for changing the rule on this selector as well?
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.
Thanks so much for the review!
It might be a remnant of me web-inspecting and trying out a few things, let me see if I can remove it.
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.
It does seem like I can remove that. So I did. Thanks again.
I rebased in hopes that the checks will now pass. 🤞
0f4893f
to
e58d519
Compare
Fixes #19610. Alternative to #19693.
A bug exists where if you have a fully saturated color, the color dropper which is supposed to sit at the far right edge, causes the parent container to shift and mess up the layout.
This PR fixes that, by allowing the eye dropper to overflow:
It also adds a focus style.