-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Cover: Add text color block support #41572
Conversation
Size Change: +95 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
0fd2ecc
to
0b6e535
Compare
0b6e535
to
b6dacd2
Compare
b6dacd2
to
d6f4583
Compare
Flaky tests detected in d6f4583. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4132436038
|
Tested well for me. It looks like the unit test failures are related to the change though. Let me know if you want me to take a look at those, otherwise ping me when they are sorted and I will give it another review. |
The Cover block previously (and likely accidentally) relied on gradient classes and styles not being serialized on the block wrapper as it didn't have text or background support. Its ad-hoc use of the same gradient attribute gives rise to the problem.
d6f4583
to
28f40f7
Compare
They were indeed. Although the problem runs a bit deeper than simply toggling on the text support as per this PR. Problem BreakdownThe Cover block's ad-hoc use of the Until the addition of text color support in this PR, the Cover block accidentally relied on the fact that it explicitly disabled text and background colors, which resulted in no gradient class being generated from that attribute. Once either text or background support was enabled, the color block supports Essentially, it assumes if a block is opting into a support feature, it won't have ad hoc use of an attribute that would normally be used for another of that support's features. An alternative example to the one in this PR might be, a block opting into background color support but implementing its own ad hoc use of the Possible solutions
Option SummaryUltimately, I went with skipping the serialization of the gradient support despite the block not actually opting into it. It's a simple fix, won't require a deprecation to remove, and we can be sure it only impacts the Cover block for now, allowing exploration of Option |
@glendaviesnz the fixtures tests have passed locally and in the e2es now after tweaking the serialization of the gradient class. I think we can get this one reviewed with a view to landing it separately to any tightening of how color block support classes and styles are generated. |
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 tested well for me:
✅ Could transform between Cover and Media/Text blocks with preset and custom colors and settings were retained as expected both ways
✅ Text color set on Cover block was inherited by child blocks in editor and frontend
✅ Text color set in theme.json or global styles worked as expected in editor and frontend and could be overridden by user applied text color
✅ Auto assigning of light and dark text worked as expected unless a text color had been applied
✅ Applying of a gradient to the Cover block overlay still worked as expected
I agree that skip serialization is the best option for this PR and updated that color support can be followed up later.
Depends on:
Related:
What?
Adds text color support to the Cover block.
Why?
This makes it easier to style the text color for a Cover's inner blocks as it can be set once only on the Cover block itself. It also allows us to clean up the Media & Text transforms so that text colors are cleanly passed between the resulting blocks.
How?
:where
to allow for global styles or theme.json supplied colors to take precedence. (Whether this is desirable could be up for discussion)Testing Instructions
Screenshots or screencast
Screen.Recording.2022-06-07.at.4.08.44.pm.mp4
Screen.Recording.2022-06-07.at.4.11.02.pm.mp4