-
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
Refactor the filters (duotone) panel as a generic UI Styles component #49577
Conversation
} ); | ||
const settedValue = duotonePreset | ||
? `var:preset|duotone|${ duotonePreset.slug }` | ||
: newValue; |
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.
If the duotone doesn't come from a preset, I'm storing an array of colors. While this doesn't work yet (not available in the UI) in global styles, it should work when you "enable the flag"
Size Change: +389 B (0%) Total Size: 1.35 MB
ℹ️ View Unchanged
|
Flaky tests detected in 7f71854. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4616957762
|
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.
In general, this refactor is testing pretty well.
I did run into the blank screen issue when color.customDuotone
and color.defaultDuotone
were set to false. I've left an inline comment on why that might be.
Other than that:
✅ Could set duotone filters globally for different block types
✅ Could override those duotone filters for individual blocks
✅ Custom duotone colors for individual blocks still work
dc86cf6
to
7f71854
Compare
So it turns out that the bug that I was tracking here: (selected duotone not shown as selected in the UI because the "duotone" variable is undefined) is actually a bug that exists in trunk as well. It's a very weird bug, basically the I tried reproducing the issue in an isolated React code sandbox but failed, so maybe it's not just the "filter" key, maybe there's some weird JS objects manipulation/mutating we're doing some where in the chain. So far, I failed to solve it though. |
Anyway, I don't think it should block this PR since the same bug is in trunk too. So this should be ready. |
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 working as intended and now I can disable the filter section completely from theme.json. Thanks for adding that to your refactor!
@oandregal @ajlende is this maybe a good reason to just choose |
No, it seems that this bug is due to the property being mutated somewhere. We should fix that problem rather than switching the name. |
Opened #49623 with the fix |
Related to this discussion #37064
This is also a prerequisite to #49428
What?
To unify the block sidebars between the global context (global styles) and the local context (block inspector), we're refactoring the global styles panels to use the same components between these two context. While the "filters panel" (duotone) is not available yet at the local level, there's no reason for its code to be different than the others and this also makes it easy to add it to the block level later on.
Testing Instructions
This is just a refactoring PR so just ensure that the "filters" (duotone) panel within the global styles of the "image" block works as expected.
Note