Table: Add colors block support #30791
Conversation
aaronrobertshaw
added
[Block] Table
aaronrobertshaw
requested review from
ajitbohra,
nerrad and
ntwb
as code owners
|
@nosolosw Do you have any thoughts on standardising retrieval of block support provided classes and styles when we are leveraging the "skip serialisation" feature? With the ability to pick and choose which inner elements we apply these block support props to, it seems like something we'll be doing more of in the near future. In this PR's case, I've leveraged the util from the button block that was extracted from the colors block support hook. |
Yeah, we need that extracted to a utility. It seems this util can be something like (Personally, as a reviewer, I welcome many small PRs rather than a big one. Small slots for reviews work best for me, as it allows me to help others while also advance my work without having to block half a day for a single review). |
| "color": { | ||
| "__experimentalSkipSerialization": true, | ||
| "gradients": true, | ||
| "link": false |
There was a problem hiding this comment.
Probably not necessary at the moment as the existing getColorAndStyleProps doesn't process link color, but it brings the question: shouldn't it?
There was a problem hiding this comment.
I believe the intent of the current getColorAndStyleProps was to only bring in what was needed for the button block. That happened to line up with what was needed here for the table block.
More than likely, a generic utility would need to take into account the current block's configuration of block supports.
|
When testing this, will need to make sure the 'stripes' block style is well tested and works as expected. Also worth testing with table headers and footers active. I did a quick test, and looks like text color doesn't seem to work for 'stripes'. |
My thinking was as more block support features are implemented and there are more cases of skipping serialization. Having a utility to retrieve the css classes and inline styles across block support features would be good if possible.
That might be over-the-top though in hindsight.
I'm happy to create a couple of PRs, extracting |
@talldan Any chance you can let me know which theme you were using when you tested? The "stripes" block style works ok for me with the custom colors. When you found it not to work, was that related to the arbitrary background color for the striped rows clashing with a selected text color?
|
|
@aaronrobertshaw I did some further testing, and I'm seeing that text colors don't work when a background color is selected with the stripes style: Using Twenty Twenty One. I think it's a theme issue, it works ok with editor theme styles disabled in the settings. Is that something you can see as well? If so, it'd be good to follow up with a trac ticket. |
From memory, I was likely using the TT1 Blocks theme. At least thats the first explanation that comes to mind as the GIF in my previous comment shows the font colors being applied correctly both in the editor and frontend. The standard Twenty Twenty One theme may not be qualifying for the I'll test further and see if I can confirm that. |
|
After further testing, I found two themes that introduced conflicting styles preventing the selected table colors from displaying correctly. These themes were:
In other themes, the text color was displayed correctly in both the editor and the frontend. These themes included:
If the approach to applying block support styles in this PR is accepted I can create any trac tickets needed so themes get updated to honour users' color selections. |
|
To get this merged, looks like it may require a rebase and reformat of the json files as per #30714. |
aaronrobertshaw
force-pushed
the
add/table-colors-block-support
branch
from
ada219b
to
ec0b6d5
Compare
aaronrobertshaw
changed the base branch from
trunk
to
add/color-block-support-utilities
Thanks for all the effort on the reviews 👍 As part of addressing the earlier suggestion to move the My apologies for the updates post approval! |
|
Size Change: +96 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
No worries! I think you could've merged and then followed up with the other change in another PR, but it's ok to do it this way as well. I'm not all that familiar with the support hook, so would appreciate if @nosolosw could give the thumbs up to that change. |
|
Thanks for taking another look at this one 👍
The color block support utilities have now been merged as well as updates to the button block to use them. Hopefully, this should only need a quick rubber stamp from here. |
| // As the previous arbitrary colors won't match theme color palettes, the hex | ||
| // value will be mapped to the style.color.background attribute as if it was | ||
| // a custom color selection. | ||
| const oldColors = { |
There was a problem hiding this comment.
We are making every table block that exists use inline styles instead of a color class. But we will still need to keep the old classes so the blocks properly work on the frontend even if they are never opened on the editor.
Could we leave the attributes as they were instead (keeping the classes and named attributes)? I guess we could keep the old colors as the default table block palette on lib/experimental-default-theme.json. Given that the classes still exist and will always be preset I guess the blocks should still look ok without any migration.
There was a problem hiding this comment.
We are making every table block that exists use inline styles instead of a color class. But we will still need to keep the old classes so the blocks properly work on the frontend even if they are never opened on the editor.
If the block's are not opened in the editor and therefore not migrated. They would still keep the old CSS classes. This is why those classes still exist within the style.css file.
Could we leave the attributes as they were instead (keeping the classes and named attributes)? I guess we could keep the old colors as the default table block palette on lib/experimental-default-theme.json. Given that the classes still exist and will always be preset I guess the blocks should still look ok without any migration.
If the table block is given its own custom color palette, as you suggested, with the old colors under the same names, their CSS classes should be unchanged. With that approach, the original backgroundColor attribute could remain as is, instead of migrating it to the style.color.background attribute.
| value: backgroundColor.color, | ||
| onChange: setBackgroundColor, | ||
| label: __( 'Background color' ), | ||
| disableCustomColors: true, |
There was a problem hiding this comment.
Previously custom colors were intentionally disabled for the table block. This PR enables custom colors for the table block. Is this change intentional? Should we keep them disabled using lib/experimental-default-theme.json?
There was a problem hiding this comment.
With the adoption of the colors block support increasing the user's control over colors including font color, it seemed like this was the preferred direction to go. More so when taking into account some of the conversation in #19659.
|
Thank you for the great feedback @jorgefilipecosta 👍 It would be great to get some input from others regarding whether we should maintain a custom color palette for the table block as suggested as well as allow custom colors. @talldan and @nosolosw do you have any ideas or preferences on this? |
|
I would defer to the conversation on #19659, where it has been made very clear that the current limited palette is a problem for users. See @kjellr's comment here: I think this PR would be a good first step, but there are additional challenges that I think it would be good to devote some time to solving:
|
|
In terms of how this uses the block supports: it's fine, although it looks like we need to rebase to get the latest utilities (and remove those changes from this PR). I suggested at #30791 (comment) that we start only with background for this PR (which works as of today, just with more theme colors) and tackle text/border separately. |
|
By the way, I've noticed the dropdown for the default style looks a bit odd (both Firefox and Chrome on Linux):
|
aaronrobertshaw
force-pushed
the
add/table-colors-block-support
branch
from
ec0b6d5
to
09766a1
Compare
aaronrobertshaw
force-pushed
the
add/table-colors-block-support
branch
from
09766a1
to
86f6423
Compare
Unfortunately, I don't have a Linux VM handy at the moment to test. It appears OK for me across browsers under Mac and Windows using a few different themes. Is it just this PR you noticed the issue with and is it still an issue after the rebase? |
Oh, sorry, hadn't tested on |
|
I've created #31261 to track the desired follow ups mentioned in #30791 (comment) |
aaronrobertshaw
added
Needs Design Feedback
Description
Replace ad-hoc background color selection for tables with colors block support.
How has this been tested?
Manually and via fixtures tests using
npm run test-unitTest Instructions
Screenshots
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).