Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix default duotone preset SVG and style generation #38681
Fix default duotone preset SVG and style generation #38681
Changes from all commits
01d593e
51d2bc5
9072348
ccb65ac
93f23af
7b4e11c
795de03
3cda90b
68cdf4e
fe1c37a
6b65616
ece841d
d47041e
bb55054
e6ceb4b
67dbd4b
10e3e26
159afc3
9de2494
1410ae1
6bdf5d0
77b04c7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
are we introducing this concept,
prevent_override
here?is there a way to reframe it in the assertive positive sense? I always have trouble understanding contradictions or negative constructions. for example, instead of
prevent_override = true
maybehide_defaults = false
?not that
hide_defaults
is certainly what we want, but it's actively what we're doing (or maybe "hide" is wrong, but we're dong something), instead of what we're not doing.I guess this is an existing term, but I still find it confusing, especially how we bring that negation into the JS code with
doNot = ! do; value = doNot ? EMPTY : value
Not a big deal, and it's in a rather complicated settings sourcing function so I get is. That's my only real feedback I think I'm able to provide here.
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.
It was kind of like that before, but I changed it hoping that it would make things less confusing despite being a negative construction. I'll explain my thought process for replacing
override
withprevent_override
.Arrays in the
PRESETS_METADATA
reference values in theme.json, so let's take this theme.json as an example. I'm usingdefaultPalette
here, butdefaultGradients
anddefaultDuotone
should behave the same way.Previously, there was a key called
override
that could be a path array or a boolean.I expected the values to resolve like this when I was reading it.
But actually, there was a function,
should_override_preset
, that would look up the value and invert it or just pass through a boolean without inverting it. So the actual substitution would look like this.As part of this PR I added
use_default_presets
which also looks up a value from the theme.json, but does not need to invert the lookup.It needs to resolve to this.
Between the weird behavior of
should_override_preset
and wanting consistency for how theme.json settings are looked up, it made more sense to me to replaceoverride
withprevent_override
and use a more genericget_metadata_boolean
for both of them.Does that make sense to you? Would you rather have inconsistent path array meanings or a negative construction? Or is there another simple option that I'm overlooking?
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.
it's too bad the JSON file has the inversion, rather than something like,
showDefaultPallette
oruseDefaultPallette
orincludeDefaultPalette
probably the confusion stems from having multiple speakers saying the same words but from different perspectives.
should
prevent_override
protect the default values, the hard-coded values, or the JSON file values?in the
PRESETS_METADATA
examples are those priority lists? or are those two separate settings, such aspalette
andgradient
etc…?I guess the idea is that if we have
true
orfalse
as the value ofuse_default_presets
or forprevent_override
then that value will be used int he end. if we set a path as an array for the field though it will read from the JSON and use that?what is the relationship between
prevent_override
anduse_default_presets
? pardon my confusion; I'm trying to learn something about this system but still feel ignorant. is there a logic table with the inputs and desired outcomes for this?My confusion peaks in lines like these. It's hard for me to read through the code and explain what impact
$skip(_default_presets)
is supposed to have on the end result.Are "defaults" and "presets" different?
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.
Lots of good questions! I'll answer them a bit out of order in a way that they can build upon one another.
A preset is the metadata needed to generate the relevant global styles parts: CSS variables for everything, CSS classes for things like text/background color, and SVGs for things like duotone filters.
They come from three different origins listed here in the order that they are processed:
In the theme.json, the presets would be the objects inside the duotone array, for example.
prevent_override
protects the slugs used by default presets.So maybe we could name it
protect_default_slugs
orreserve_default_slugs
or something? Would that make more sense?The
override
option was added in #36811 and updated to use a path in #37008. tl;dr It was added to fix the UI by skipping theme slugs that already existed in the default palette. Theme developers that had"defaultPalette": false
in their theme expected to be able to use these reserved default slugs, so it was then tied todefaultPalette
.These screenshots from #36811 illustrate it well—
black
andwhite
are duplicate keys in TT1-blocks and were colored hot pink#FF69B4
for emphasis.Let's run through an example with a slightly modified theme.json from the #36811 example to see what is being generated from the presets.
Here are the relevant CSS variables that are generated.
Notice that the
white
andblack
values we defined in theme.json do not get generated. The value for--wp--preset--color--white
is#ffffff
instead of#FF69B4
. WhendefaultPalette
istrue
, the default colors take precedence.My expectation as a block theme creator would be that values I pass replace the default ones, but the expected outcome according to #36811 and #37008 is the other way around.
With
"defaultPalette": false
, this is what should be generated now.prevent_override
controls how theme presets with the same slug as default presets should be handled as explained above.use_default_presets
was added by this PR to wholesale prevent generating anything from default presets (including the SVGs in question) when its value isfalse
. My take was that the color/gradient/duotone palettes should all behave the same, so I fixed the default generation for them as well.In the case of the color/gradient/duotone palettes. I suppose you could say that
use_default_presets
obsoletesprevent_override
.However, in the case of
fontSizes
andfontFamilies
, the values differ. The default values should always generate and the theme preset slugs should override the default preset slugs like they did before.use_default_presets
prevent_override
false
true
false
true
true
They are separate settings. You could add duplicates to the list, and I expect it would generate duplicates in the output.
However,
ALLOWED_ORIGINS
is kind of a priority list with the exceptions noted above for thedefault{Palette,Gradients,Duotone}
option in theme.json.Exactly.
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.
that's extremely helpful and clarifying @ajlende, thank you for taking the effort to add that.
Oh no! those names make so much more sense to me 😆. I trust there was a good reason to diverge from them.
please don't let this derail the PR or the fixes, it's me doodling on the idea.
it's almost like we're looking at a natural hierarchy and talking about what behavior happens when finding duplicate values. the order will always be
core
(default) applies first,theme
second, anduser
third. at any point in the hierarchy we can choose to ignore a new duplicated value and let the previous assignment remain or replace the previous assignment with our new value.the kicker is that sometimes we don't want
core
values enter into the equation at all. we want a blank slate.❓ is there any reason a
custom
config couldn't start clean as well and wipe out all of the previously-assigned values?I wonder if this description could account not only for
use_default
but alsoprevent_override
and make it work at both the theme and the custom levels. a theme could decide toresetConfig
and the custom level could extend that.on the other hand, it couldn't bring back the default levels with this description since the theme might have already reset them.
my other idea was wondering if we are standardized on
default
,theme
, andcustom
, if we could go ahead and set the priority.while a bit more verbose this would give each level in the hierarchy the control to decide which values should appear and which should take precedence in a conflict. if a theme turned off the default values then a custom config could turn them back on. a custom config to let the defaults stay but wipe out the theme's colors.
any thoughts? way too much discussion about something that's already embedded in the system? my noodling is mostly around thinking through what exactly the behaviors are we're doing and avoiding special-casing in the interface, such as when calling out
default
inuse_default
.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'm happy to explain the existing system and how it drove my decisions for this bug fix, but, yeah, we're starting to get into big picture changes with some of these. I'm just trying to make sure SVG filters don't get left behind when changes are made to colors/gradients/etc. and, in this case, fixing a bug that is more obvious because of SVG filters.
Still, these questions might be interesting for @oandregal and/or @jorgefilipecosta to see since they are more involved in global styles than I am.
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.
here's an example of the confusion:
$override_preset
reads to me like a directive, such as "you should override the preset" whileprevent_override
sounds more like an allowance, "you may or may not override the preset."two changes that may not be ideal but which would remove the confusion from this line would be to:
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 does seem like a very nice improvement over
should_override_preset
because the conditional inversion is surprising.