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

ColorPicker: fix layout overflow #42992

Merged
merged 4 commits into from Aug 9, 2022
Merged

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Aug 4, 2022

What?

This fixes a layout issue with ColorPicker, where the "copy to clipboard" button would end up being rendered further right than intended, causing the whole picker to scroll when embedded inside a popover

Why?

Fixing a layout bug.

How?

The issue can be fixed by adding the box-sizing reset to the specific wrapper, although we should probably understand if we should apply the fix in a more generic way (i.e. to the HStack or even the Flex components) (cc @mirka )

Testing Instructions

In Storybook:

  • look at the "copy to clipboard" button in the ColorPicker component, and make sure that its parent wrapper doesn't extend beyond 100% width of its container.
  • in the GradientPicker example, click on color stops in the gradient bar, and make sure that the popover with the colorpicker doesn't have horizontal scrollbars

Screenshots or screencast

trunk this PR
Screenshot 2022-08-04 at 22 08 37 Screenshot 2022-08-04 at 22 17 55
Screenshot 2022-08-04 at 22 08 57 Screenshot 2022-08-04 at 22 08 24

@ciampo ciampo requested a review from ajitbohra as a code owner August 4, 2022 20:18
@ciampo ciampo self-assigned this Aug 4, 2022
@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Aug 4, 2022
@ciampo ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Aug 4, 2022
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

This tests well for me 👍

✅ Unit tests pass
✅ No horizontal scrollbars in Storybook examples
✅ No change in editor (no scrollbars on trunk or this PR)

While testing, I encountered another minor issue with the ColorPicker appearing "under" the select controls in the GradientPicker story. At least until it is focused. This appears to be unrelated to this PR as it's also happening on trunk.

Demo of z-index issue
Screen.Recording.2022-08-05.at.2.09.27.pm.mp4

@ciampo
Copy link
Contributor Author

ciampo commented Aug 5, 2022

This tests well for me 👍

Thank you for your review! Let's wait for @mirka ' s opinion on whether this fix is appropriate, of if we should look into adding the box-sizing styles directly to the HStack or even the Flex component.

While testing, I encountered another minor issue with the ColorPicker appearing "under" the select controls in the GradientPicker story. At least until it is focused. This appears to be unrelated to this PR as it's also happening on trunk.

Interesting that you flagged it! I believe that's been around for a while (I believe at some point it was pointed out in a convo with @andrewserong too), but I also came across it yesterday and decided to open an issue with a list of improvements to GradientPicker (& related components):

In particular, the color picker appears "under" because the gradient bar has a style of opacity: 0.4 applied to it when there's no gradient set. That opacity CSS rule implicitly creates a stacking context which makes the color picker render below the controls. But I will also note that, even if the stacking problem was not around, the popover would still look weird at opacity: 0.4 — I believe that we should just come up with a different visual state for when a gradient is not set

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

I don't have a great answer. To be honest the more I think about it, it's kind of complex. But in general, for now I'm inclined to put the box sizing reset on the root-level wrapper of the component, which is ColorfulWrapper in this case.

The concerns I think we need to juggle are:

  • Easier is better. Not having to manually add the reset in a ton of places is good.
  • Less nesting of resets is better. From what I could gather, I think we should be fine in terms of performance, but nesting too many * glob rules seems... a bit messy. Of course, some nesting is unavoidable. But this is why I would prefer not to add the reset to components like Flex, which tend to be nested a lot.

@ciampo ciampo force-pushed the fix/color-picker-layout-overflow branch from 8815915 to 42d5c4d Compare August 8, 2022 17:37
@ciampo
Copy link
Contributor Author

ciampo commented Aug 8, 2022

Makes sense. I've followed your advice and moved the box-sizing reset to ColorfulWrapper

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

This is still testing well for me. 🚢

@ciampo ciampo merged commit a7f569c into trunk Aug 9, 2022
@ciampo ciampo deleted the fix/color-picker-layout-overflow branch August 9, 2022 11:31
WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Aug 9, 2022
@github-actions github-actions bot added this to the Gutenberg 13.9 milestone Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants