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

GradientPicker: remove padding and disable overflow of color picker popovers #55265

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Oct 11, 2023

What?

As flagged by @t-hamano in #55149 (comment), popovers containing the ColorPicker in the GradientPicker component include some padding around the color picker. This PR removes the padding (and overflow: hidden styles) to have the popover look more uniform with other similar color picker popovers.

Why?

UI polish and consistency

How?

  • Remove some ColorPicker-specific styles
  • Introduce the resize: false popover prop (which also causes overflow: hidden styles to be applied)
  • Use the DropdownContentWrapper component to easily remove the popover's internal padding

Testing Instructions

  • Open the GradientPicker Storybook example
  • Click on the color stops on the gradient bar
  • Notice how the popover containing the gradient picker doesn't have internal padding, and how the drag handle correctly overflows its parent

Screenshots or screencast

Before (trunk) After (this PR)
Screenshot 2023-10-11 at 16 52 07 Screenshot 2023-10-11 at 16 53 26

@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Oct 11, 2023
@ciampo ciampo self-assigned this Oct 11, 2023
@ciampo ciampo changed the title Remove padding in color picker popover GradientPicker: remove padding and disable overflow of color picker popovers Oct 11, 2023
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

It's working as expected, but I'd like to leave a few comments.

packages/components/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -81,7 +81,6 @@ export const ColorfulWrapper = styled.div`
align-items: center;
width: 216px;
height: auto;
overflow: hidden;
Copy link
Contributor

@t-hamano t-hamano Oct 11, 2023

Choose a reason for hiding this comment

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

If this style is removed, an unintended scroll bar will appear when I move the pointer to the right edge of the regular color picker. This problem can be reproduced on Windows, but it might be possible to reproduce it on MacOS by changing the scroll bar settings.

However, strangely, this problem does not occur in #55149, where the same style has been removed.

7eb7c82e55ad7e1c86ebb3999f2e2f63.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's because it's missing the change to packages/components/src/color-palette/index.tsx setting resize: false for that specific instance of dropdown.

This can be confirmed by removing the resize : false change in this PR in the packages/components/src/custom-gradient-picker/gradient-bar/control-points.tsx file, and noting how the popover in GradientPicker will also start scrolling.

In short, I think this is behaving as expected.

@ciampo ciampo requested a review from t-hamano October 11, 2023 19:33
Copy link
Contributor

@brookewp brookewp left a comment

Choose a reason for hiding this comment

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

Looks great! 🚀

@ciampo ciampo force-pushed the fix/color-gradient-color-picker-dropdown-styles branch from ab5ecbc to f1019df Compare October 12, 2023 08:05
@ciampo ciampo enabled auto-merge (squash) October 12, 2023 08:12
@github-actions
Copy link

Flaky tests detected in 381fe99.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6493032039
📝 Reported issues:

@ciampo ciampo merged commit 1e58f72 into trunk Oct 12, 2023
50 checks passed
@ciampo ciampo deleted the fix/color-gradient-color-picker-dropdown-styles branch October 12, 2023 09:03
@github-actions github-actions bot added this to the Gutenberg 16.9 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants