-
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
Update: Show all color and gradient origins (core, theme, and user). #35970
Update: Show all color and gradient origins (core, theme, and user). #35970
Conversation
Size Change: +1 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
Hey, I've given this a run and this is what I see: core + theme palette👌 I've tried core and theme palette for some themes and it's working well for these ones:
This stems from the fact that user styles❓ For user palette, this is what I see when the user changes the color value of the "black" color to set it to red-ish (silly thing, I know, but it works for stress testing :) ): 👌 Then I reset the changes to start over and the user palette is no longer shown. ❓ The other test I've done is what happens when the user adds a new color (reset the changes above and start fresh): ❓ Finally, I've switched themes and verified that the user colors are cleared because they are tied to the theme. This raised a couple of questions around how do we expect this to work for users:
blocks that don't use block supportsWithout having done a deep dive into the code yet, I've noticed this updates a few components so a number of blocks will have the new design:
I've noticed that there are some other blocks, such as navigation, that don't use the color support mechanism and use the |
This bug is independent from this PR and I've got a fix at #36054 |
23d8d1c
to
f4b9d07
Compare
This PR got some conflicts, and I rebased it. Hi @oandregal, Thank you for the review! What this PR does is show the three-color origins when available. The strange behavior noted when changing a theme color or adding a user color is because the color palette editor assumes the default value of user colors are the theme colors, so the duplication happens. We are changing that logic; the user colors will be an empty pallet at the start in #35783. So after #35783 is merged, no more duplication is going to happen.
Nice catch 👍 PanelColorSettings is updated in the sense that it just uses PanelColorGradientSettings. The issue was that some blocks were not yet passing the flag to enable the multiple origins and is now fixed. |
f4b9d07
to
7588956
Compare
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 works as advertised for me.
Code-wise, I haven't done a deep dive into this, and I'd appreciate it if people with recent components experience would be able to take a look. Given the time constraints for the freeze, it can also be done as a post-review and refactor later.
We need #36173 that fixes a recent regression in trunk
for user colors to work, but that's unrelated to this PR.
@jorgefilipecosta I've found a minor thing: when the color selected is white, the color HEX is not visible |
@@ -215,3 +216,59 @@ export function getSupportedGlobalStylesPanels( name ) { | |||
|
|||
return supportKeys; | |||
} | |||
|
|||
export function useColorsPerOrigin( name ) { |
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 wonder if these would be better on their own "color" file (leaving hooks for the raw hooks only)
disableCustomColors={ ! areCustomSolidsEnabled } | ||
__experimentalHasMultipleOrigins |
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.
What would it take to make this the default and remove the prop entirely?
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.
Hey @jorgefilipecosta , this looks awesome! It's so cool to see the Global Styles UI migrate to the new components!
A bit late to the party, but would it be possible to document the changes made to the @wordpress/components
package? It'd be good to add at least entries to packages/components/CHANGELOG.md
.
cc @mirka
@jorgefilipecosta is there a way to disable/hide the CORE color palette? |
You can follow this issue #36470 |
Opened #36448 to tackle this follow-up |
This PR changes the color components in the block editor inspector and the global styles to show multiple colors and gradient palettes.
The idea of this PR is that the base components ColorPalette and GradientPicker are changed so the color and gradient props are now an array of color sets with the following format: colors = [ { name: "Core", colors: [..] }, { name: "Theme", colors: [..] }, ... ]. That format is optional and only happens when the property __experimentalHasMultipleOrigins is true, so we are totally back-compatible.
How has this been tested?
I used a background color on the cover block from different origins and verified things worked as expected.
I used a background gradient on the group block from different origins and verified things worked as expected.
I applied a global text, background, and link, color from different origins, and verified things worked as expected.
Screenshots
cc: @jasmussen, @pablohoneyhoney would you be able to give a design/UX review to this PR?
cc: @ciampo, @mirka as people components work.