Extract a ColorPanel component as a reusable component between Global Styles and Block Inspector #48893
Conversation
youknowriad
added
[Type] Code Quality
| ) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
I was surprised that I had to make changes to this file to make the "hover" colors styles work in the frontend. I thought they'd just work since we support them for Global Styles. I guess I'll have to do similar changes for heading and button elements which makes me think that there's probably a better way to avoid all this logic and rely on some existing code (style engine or whatever)
There was a problem hiding this comment.
The style engine is currently naive about the selector to use, so it takes a style object and passes back styles using the selector it's given. This created a fairly clean separation between block supports and the style engine, while containing scope for the style engine project.
Perhaps, a bit like the typography block support, we could have a function in elements.php that is responsible for dealing with each of the potential element selectors, and then calls the style engine. At the moment that's this function, but it could probably be extended to deal with potential future elements. And we could then use that function in both global styles and at the block level.
For the moment, though, inlining an additional call to the style engine like this sounds good to me!
There was a problem hiding this comment.
Maybe the style engine had two methods. The current naive one and what that takes a full "style" object and generate everything including selectors.
There was a problem hiding this comment.
that takes a full "style" object and generate everything including selectors.
Good point — it'd be worth exploring!
| @@ -163,7 +163,6 @@ export function BorderPanel( props ) { | |||
| <StylesBorderPanel | |||
| as={ BordersInspectorControl } | |||
| panelId={ clientId } | |||
| name={ name } | |||
There was a problem hiding this comment.
These were just leftovers.
| @@ -6,45 +6,20 @@ import { useState, useEffect } from '@wordpress/element'; | |||
| /** | |||
| * Internal dependencies | |||
| */ | |||
| import ColorGradientSettingsDropdown from '../components/colors-gradients/dropdown'; | |||
There was a problem hiding this comment.
Unfortunately, I can't remove the old components as they're exposed and used in several core and third party blocks.
| ? props.attributes.style?.elements?.link?.[ ':hover' ] | ||
| : undefined, | ||
| selector: `.editor-styles-wrapper .${ blockElementsContainerIdentifier } ${ ELEMENTS.link }:hover`, | ||
| }, |
There was a problem hiding this comment.
Same as https://github.com/WordPress/gutenberg/pull/48893/files#r1128026980 Feels like this should be somehow part of style engine or whatever.
|
Size Change: +971 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in efebb78. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4499088360
|
|
I've added the "headings" and "buttons" colors to the color panel component. I noticed a problem though, for a reason that I can't explain, the "heading" and "button" buttons are not showing up. If I remove them from the "DEFAULT_CONTROLS", I can start adding them from the more menu but if they're made default controls, they are not shown at all. I think it's a bug in |
|
Thanks for tackling the colors panel @youknowriad, it's always been rather convoluted. When this PR is ready for final reviews it might help to have a bit of a breakdown around the new approach, what changes were made and why etc.
The One possibility to guard against this might be to update the ToolsPanelItem to check for the item's property rather than the property In addition, there were a couple of missing styles and props for the global styles panel. I've taken the liberty to push a couple of commits to address these. Please feel free to revert or change anything you'd like to improve. I can give this PR a closer look later this evening or early next week. Nice work so far! Screen.Recording.2023-03-10.at.11.30.19.am.mp4 |
youknowriad
force-pushed
the
add/color-panel
branch
from
13fb6b7
to
d1724b9
Compare
|
I've rebased the PR and added the new caption element. This is getting very close and I'd appreciate some reviews here. Thank you ❤️ |
|
Nice work. Taking this for a spin just to compare with trunk, I'm focusing mostly on Global Styles → Colors. Text appears the same. Buttons appears the same. Headings appears the same.
It's coming into clarity that the text/heading hierarchy on this page is not that considered. The tabs for heading background are also awkwardly indented. So there are some larger design tweaks we can do, separately. I think I'm missing something, though, because for links I see hover both in this PR and in trunk:
I'm also seeing "Elements" in the main GS panel, instead of "Color" as Aaron appears to see. Did I test the PR right?
|
|
@jasmussen yeah I think there's something missing in your tests, are you sure everything built properly and all? |
|
Oh, I found it. In some testing I had deactivated the gb plugin. My apologies, as you were. Here it's working:
Generally looking good. It's okay that these don't also have the palette picker drilldown, so long as the Palette option already exists. In fact on a separate note, we should only have the palette visible in GS > Colors, not in GS > Blocks > [Blockname] > Color:
Two small glitches. Visible in the GIF, occasionally I could get the flyout to open in an awkward place. It seemed to be when clicking on or about the swatch inside the itemgroup:
Not sure if that's from this branch or general. Otherwise, this separator: Either it should go edge to edge left and right, or it should just be removed. Both would be fine. |
I noticed the separator, too. Looks like it might be unintentional for it to be there in global styles? |
There was a problem hiding this comment.
Great work here so far! Aside from what Joen mentioned above, it's testing quite well for me. It took me a little while to find the h1 through h6 controls for adjusting colors for each of the different headings, but once I did, I really liked how compact it looks seeing them all lined up in the one list 👍
Just looking into some of the e2e failures, and it looks like the style variations test failures are because of losing the aria-label on the background color button elements.
Here's the visual difference between the two panels in trunk vs this PR
| Trunk | This PR |
|---|---|
 |
 |
And here's the difference in markup for the buttons
Trunk:
This PR (missing aria-label):
The test is currently looking for the text Colors background styles, but perhaps this could be updated to Background since there's no aria-label now? Or should we look at restoring the aria-label?
There was a problem hiding this comment.
Thanks for the continued effort here @youknowriad 👍
Ok. I've addressed all the issues I think aside the popover one.
I've given this another test.
All but one of the issues I raised in my last review has been resolved.
❓ Reset all now clears heading, button, caption etc styles. It doesn't however reset link hover colours correctly in either the block inspector or global styles.
✅ Contrast checker now displays when making a poor contrast selection and clears appropriately.
✅ The diagonal slash through empty color indicator is now shown correctly
✅ Incorrectly positioned color popover is appearing on trunk for me and not introduced in this PR.
| Block Inspector | Global Styles |
|---|---|
Screen.Recording.2023-03-24.at.10.45.25.am.mp4 |
Screen.Recording.2023-03-24.at.10.47.31.am.mp4 |
Oh yeah, forgot about that one, thanks. It should be a quick fix. |
|
The link hover reset is fixed now. |
|
Opened #49349 to track the Popover glitch |
There was a problem hiding this comment.
Just took this for another spin, and it's looking in quite good shape! I only found one real issue, which looks like it should be a fairly simple fix 🤞
✅ Disable settings.color.heading works correctly
✅ Disable settings.color.button works correctly
✅ Disable settings.color.caption works correctly
✅ ContrastChecker now behaves as on trunk (it's a little flaky, but doesn't appear to be any flakier than on trunk)
✅ Setting / resetting individual color controls / reset all at the block level appears to work
✅ Setting / resetting individual color controls / reset all at the elements level in global styles appears to be working
✅ Setting / resetting individual color controls / reset all at the block level in global styles appears to be working
❓ The only real issue I encountered was with the color indicator not displaying gradient color for Background when set. This appears to be because TwentyTwentyThree theme sets a background color, so when a gradient is set, both values exist, and the background color is preferenced in this PR for the indicator over the gradient color. On trunk, I think it's the other way around.
Other than that, after reading through the code again, and with the known issue with the popover, I think this PR's looking in good shape! It's a challenging one to review given the size of the change, so it'd probably be good to get another approving review before landing it, just in case I've missed anything.
Also, there's probably an opportunity to consolidate or split out some of the logic in the ColorPanel component in follow-ups, as it's quite a long component. My main note there is that it might be good to see if we can improve the readability to make it easier to make subsequent changes.
For now, though, great work on all the consolidation efforts here, and for the persistence across each of these components and panels! ✨
| ) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The style engine is currently naive about the selector to use, so it takes a style object and passes back styles using the selector it's given. This created a fairly clean separation between block supports and the style engine, while containing scope for the style engine project.
Perhaps, a bit like the typography block support, we could have a function in elements.php that is responsible for dealing with each of the potential element selectors, and then calls the style engine. At the moment that's this function, but it could probably be extended to deal with potential future elements. And we could then use that function in both global styles and at the block level.
For the moment, though, inlining an additional call to the style engine like this sounds good to me!
packages/block-editor/src/components/global-styles/color-panel.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/color-panel.js
Outdated
Show resolved
Hide resolved
| @@ -86,13 +86,13 @@ test.describe( 'Global styles variations', () => { | |||
|
|
|||
| await expect( | |||
| page.locator( | |||
| 'role=button[name="Colors background styles"i] >> data-testid=background-color-indicator' | |||
| 'role=button[name="Background"i] >> .component-color-indicator' | |||
There was a problem hiding this comment.
Why do we have to use class name selectors here? Could we use role selectors or data-testid instead?
There was a problem hiding this comment.
I was expecting this comment :)
I preferred classnames for two reasons here:
- First one, I don't like data-test-ids attributes when they linger on production code. I think we should avoid changes in production code that are only meant for testing. One potential solution would be to strip these in non test builds but not sure how to do that.
- Second one is that the change in the structure of the component (more generic) means we can't specifically say that a particular indicator is "background" or other "text" unless we add extra props and extra complexity that I was not happy to add for test purposes.
There was a problem hiding this comment.
Let's not start a conversation here about whether data-testid is a best practice in tests. We already have best practices guidelines committed to the repo, as well as all the tooling recommendations in the community (testing-library, playwright, etc), so I would expect us to follow them here :). You are more than welcome to open an issue or discussion and we can discuss further and rethink our best practices there!
Second one is that the change in the structure of the component (more generic) means we can't specifically say that a particular indicator is "background" or other "text" unless we add extra props and extra complexity that I was not happy to add for test purposes.
I'm afraid that I don't follow, could you provide me an example code of what you mean here?
There was a problem hiding this comment.
We pass an array of colors to the indicators component and we don't specificy which color is which. So there's no way I can say which indicator is background or text here.
There was a problem hiding this comment.
I wanted to open a PR to update the data test id best practice and I don't see it mentioned anywhere. Did I miss anything? Also, it seems it's the only place where it was used. So I guess now there's nothing to change for this?
There was a problem hiding this comment.
This UI is the same UI we already had in the block inspector so I don't expect the UI to be completely inaccessible, that said a small bug is possible. I can reproduce the fact that the focus doesn't seem to jump within the popover when clicked. That said the label is there because buttons use the text content as label when there's no aria-label and I believe the text in these buttons is good enough to avoid aria-labels.
I'll look at the focus issue.
There was a problem hiding this comment.
That was a bug in the TabPanel component that should be fixed with this PR #49368
There was a problem hiding this comment.
Thanks, appreciate the quick fix. I've merged that PR.
That said the label is there because buttons use the text content as label when there's no aria-label and I believe the text in these buttons is good enough to avoid aria-labels.
Accessibility is at least a bit better now that the popover content can be focused to check what the current color is.
It doesn't necessarily need an aria-label, I was thinking more along the lines of some extra hidden text (<VisuallyHidden>(current color: #FFF)<VisuallyHidden>). At the moment it'd be a bit of a frustrating process to determine which colors are used for each element if a user can't see them.
There was a problem hiding this comment.
@talldan Yeah, I think that could be a nice addition, but the only labelled item is the whole button, so we need to generate a long description, something like:
<VisuallyHidden>
text color is #FFF, background color is #000, hover text color is #333 and background hover color is #666
</VisuallyHidden>
It needs to also understand gradients. So not a small addition.
There was a problem hiding this comment.
Good point about gradients 😬
Right! I actually was able to reproduce this issue on trunk too, it's fixed now on this branch. |
|
Thanks for the reviews folks, I'm going to merge this PR as I plan to work on small changes on top of it. I'm happy to adapt in follow-up PRs if needed. I really appreciated the reviews as always. |
| style: cleanEmptyObject( updatedStyle ), | ||
| textColor: textColorSlug, | ||
| backgroundColor: backgroundColorSlug, | ||
| gradient: gradientSlug, |
There was a problem hiding this comment.
Are we purposely setting all the attributes at once in order to clean out any non-applicable ones, even if the block doesn't support those attributes? Asking because the question arose in #53941.
There was a problem hiding this comment.
I think that's the case but to be honest, it's an old PR so no precise memories at the moment.
Related to #37064 and #47348 and similar to #47356, #48636 and #48070
What?
A year ago now, I've opened a discussion in #37064 that proposes unifying the components that are used in the block inspector and global styles. The idea is that we'd have components that look like this:
That component would render a panel used to edit a given style (say "typography" or "colors"), the
valuehas a well specified prop (that is mapped over the "style" object of a block or a global style "node" ). The idea is that we could use that exact same panel in both places, the only difference is a wrapper that map the "value" to the right place:Recently we realized this vision for the TypographyPanel, BorderPanel and DimensionsPanel and the current PR does the exact same thing for the ColorPanel and uses the new UI component for both inspector controls and global styles.
ColorPanel is the last panel to migrate but also probably the most complex one, unfortunately this PR turns out to be a bit big, and it's unclear whether we can split it into smaller PRs.
Since the goal of this PR is also to bring parity with Global Styles, It ended up including a new feature: You can now set "hover" colors for links within the block inspector.
To do
ItemGroupandItemand find a way to compose these components withinPanelToolsandPanelToolsItem. This would allow us to remove a bunch of CSS and classnames but I haven't found a decent way to do that. Maybe someone familiar with these PanelTools component could help here.Testing Instructions