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
Separator block: refactor to use color block supports #38428
Separator block: refactor to use color block supports #38428
Changes from all commits
32eb898
e59364e
3452d35
c914b84
d9dfdb1
946a00b
72bc595
620a2be
6cc4459
61672a5
bc6882e
e8d02a1
c6ad0db
0a73535
bd6f912
6457a8a
ba46d5e
e5cbdef
245edc8
9248a09
bc2cacd
ea1b9b1
20cf8f1
91487e0
e4ef3d3
22ff3a2
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.
I like externalizing the deprecated styles. Hope it becomes a convention. Much clearer to read.
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.
Is there a way to deal with the specificity issue other than using
!important
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.
I will take a closer look next week and see that is possible.
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.
@oandregal I looked into this and couldn't see a way around it as the color supports is already adding
! important
in other places:The only potential option I could see is if we could remove the backGround support when
is-style-dots
is selected and just apply color instead, but this would remove thehas-background*
classes which could break backwards compat as all the existing blocks have both text and background classes applied, and themes may be relying on these.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.
The issue seems to be that this block does things a bit differently depending on the kind of style variation applied. As I understand it, we don't need the automatic classes (
.has-<slug>-color
, etc) attached to the block wrapper in all the states of this block (when the dot style variation is applied).How about using __experimentalSkipSerialization in the color block supports of the
block.json
and removing the!important
s? I see that this PR is already doing some custom things in theedit
andsave
functions, as well as the block stylesheet. When we do so, is because the normal default block support mechanism is not working as we want, so should look at disconnecting the automatic style serialization.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 would be good to get this PR in: Block Supports: Allow skipping serialization of individual features #36293.
That way we can choose specific block supports to "skip". Might mean less work, that not having to manually apply
attributes.styles
, later if we add block supports to the Separator.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.
@oandregal yes, ideally the
.has-<slug>-color
classes should just be applied when the block is in the relevant state, eg.background-color
when it is standard andcolor
when it is dotted, in which case skipping serialization would make sense.Unfortunately, the block currently applies both classes in both instances, and we have no idea how theme authors might be relying on this current combination of selectors. This means that moving to using the single selector in the specific instances would be a breaking change. I am not sure that the value gained from this change would be worth the potential impact on themes?
Also, the
background-color: none ! important
is still going to be needed in order to make sure that themebackground-color
is overridden when the dotted style is selected as noted here. This has always been included in the block, this additional setting was only added to make sure it was still applied in the editor with the addition of the new color supports settings.For the above reasons I think we should leave the classes are they currently are and leave the
background-color: none ! important
in place, but open to suggestions for ways that we can remove it while avoiding a breaking change, or also open to feedback on whether this change is important enough to warrant a breaking change.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 a hard one.
The background classes don't make sense on the dots style, but yeah... they're there ,and some folks might be counting on that. Avoiding pain for users and theme developers seems like it should take priority.
Could we deprecate and migrate in a follow up PR?
On a higher level, we've chosen, for
whatever reasons^, to use!important
in various places to ensure users' style choices take precedence.I think the struggle is how to prioritize these preferences, in this case a user-elected style vs a chosen background color. The question might be, "which
!important
is moreimportant
here"? Does that sound reasonable?We probably have to deal with it as best as we can until we find a way to mitigate such situations.
^ Edit: I got a bit wiser after reading some of the context
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.
Yeah, I understand the concerns you've brought up: that change alone would need wider testing in themes. The original PR didn't clarify why it added the both classes in every state. It's fine by me to leave it as it is in this PR and explore its removal in a follow up.
Regarding the other thing I've mentioned: I still think we need to use
__experimentalSkipSerialization
in thecolor
support since we're already managing the serialization by the block.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.
Good point @oandregal, doing that makes it a bit clearer that this isn't a fully standard use of the color supports even though the background colors are being applied correctly via serialization. I have made that change now.
This file was deleted.