-
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
CustomGradientPicker: improve initial state UI #49146
Conversation
Size Change: +117 B (0%) Total Size: 1.34 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.
Seems good to me. The main visible change is the transparency one. I think we might separately want to revisit that texture, there were some contexts in which it didn't work too well. But seems fine to go with for now as it's at least the same texture.
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 and tests well to me. I did leave one question in the inline comments.
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.
Nice, I like this change a lot. Really simplifies parsing what the utility is actually accomplishing.
// Control points color option may be hex from presets, custom colors will be rgb. | ||
// The position should always be a percentage. | ||
const controlPoints = gradientAST.colorStops.map( ( colorStop ) => { | ||
return { | ||
color: getStopCssColor( colorStop ), | ||
// Although it's already been checked by `hasUnsupportedLength` in `getGradientAstWithDefault`, | ||
// Although it's already been checked by `hasUnsupportedLength` in `getGradientAst`, |
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.
I notice you've changed this comment, but the function returning gradientAST
is still named getGradientAstWithDefault
. Did you mean to rename the function now that it's handling default gradients differently?
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.
I did rename the function at some point, but then I changed my mind and restored the original name. Although I forgot to update the comment — thank you for catching it!
af93646
to
3e2dea4
Compare
Flaky tests detected in 3e2dea4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4449184008
|
What?
Requires #48929 to be merged first
Part of #42994
Improve
CustomGradientPicker
's initial UI state and interaction:Why?
Improve the component's UX
How?
div
element dedicated to display thebackground-color
, and applying some CSS changes to force the controls on the bottom row to render with a lower stacking index than the gradient barhasGradient
logic: previously the component would think that it didn't a gradient set if, as a coincidence, the user chose a gradient value that was the same as the default gradientTesting Instructions
Interact with the component on Storybook and in the editor (for example, in the Site Editor > Global Styles Sidebar > Colors > Palette > "Gradient" tab > Create a new custom gradient)
trunk
Screenshots or screencast