ToolsPanel: Add CSS classes to first and last displayed ToolsPanelItems#37546
ToolsPanel: Add CSS classes to first and last displayed ToolsPanelItems#37546aaronrobertshaw merged 11 commits intotrunkfrom
Conversation
|
Size Change: +138 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
I was testing this both using this standalone PR, and rebased on #34027, and can confirm that the I tested using the Typography, Border and Color panels. The Similarly, removing optional controls resets the last class to the last control for me 👍 Where there is one control only both classes
Is that intentional? I don't see any side effects of this, but just checking in case there could be conflict between first and last styles when applied to the same element.
Do you mean because it's not using React Testing Library queries? I'd tend to think it's okay since the classnames are fairly invisible to the user anyway. |
ramonjd
left a comment
There was a problem hiding this comment.
This LGTM.
I left a couple of questions, also I had a question about the use of first and last classnames over at https://github.com/WordPress/gutenberg/pull/34027/files#r779209344
They seem pretty generic to me, but I might not have the entire picture.
All things I think could be dealt with later.
It was intentional. The use case I was focused on was achieving the same styling of the color dropdowns in the color panel after switching to the
Yes, it felt a little rough to me that I couldn't retrieve the desired element directly through React Testing Library queries. Other PRs have been warned against using |
That's a good point. It should be a trivial change so I can make that shortly. |
Thinking a little more on this, I'm not sure what the best approach is. More specific class names such as @ciampo would you be able to provide some wisdom on the best approach here? |
Would using a fragment like this be better/more acceptable? <ToolsPanelItem { ...controlProps }>
<>Item 2</>
</ToolsPanelItem>
...
expect( screen.getByText( 'Item 2' ) ).toHaveClass( 'first' ); |
ciampo
left a comment
There was a problem hiding this comment.
Hey @aaronrobertshaw and @ramonjd , thank you for the ping (and happy new year!)
I've left a few inline comments, sorry for the delay in reviewing!
5b3ae82 to
4a3b43a
Compare
|
I know there are ongoing discussions, but I just took this for another test spin and, functionally, it's all working as expected, similar to the last round of testing I also checked every storybook example to make sure the classes are applied 👍
|
|
The snapshot test appears to be failing on this PR (but passes locally) due to the styled components' class names changing. I've run out of time today but will revisit this tomorrow. Any suggestions on how best to handle this are welcome 🙂 |
packages/components/src/tools-panel/stories/tools-panel-with-item-group-slot.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
By using ItemGroup's isBordered and isSeparated props, we don't need to define custom CSS for borders (apart from a one-line override for the last item)
see diff
diff --git a/packages/components/src/tools-panel/stories/tools-panel-with-item-group-slot.js b/packages/components/src/tools-panel/stories/tools-panel-with-item-group-slot.js
index 950a3c3f5e..ae4eaad17a 100644
--- a/packages/components/src/tools-panel/stories/tools-panel-with-item-group-slot.js
+++ b/packages/components/src/tools-panel/stories/tools-panel-with-item-group-slot.js
@@ -169,6 +169,8 @@ export const ToolsPanelWithItemGroupSlot = () => {
<Panel>
<ToolsPanelItems.Slot
as={ ItemGroup }
+ isBordered
+ isSeparated
className={ slotWrapperClassName }
resetAll={ resetAll }
/>
@@ -218,29 +220,18 @@ const SlotWrapper = css`
const ToolsPanelItemClass = css`
padding: 0;
- border-left: 1px solid rgba( 0, 0, 0, 0.1 );
- border-right: 1px solid rgba( 0, 0, 0, 0.1 );
- border-bottom: 1px solid rgba( 0, 0, 0, 0.1 );
-
- &&.first {
- border-top-left-radius: 10px;
- border-top-right-radius: 10px;
- border-top: 1px solid rgba( 0, 0, 0, 0.1 );
+
+ &.first {
+ border-left: 2px solid blue;
}
&.last {
- border-bottom-left-radius: 10px;
- border-bottom-right-radius: 10px;
+ border-right: 2px solid red;
+ border-bottom-color: transparent;
}
&& > div,
&& > div > button {
width: 100%;
- border-radius: inherit;
- }
-
- /* .components-dropdown class overrides ToolsPanelItemPlaceholder styles */
- &[class*='ToolsPanelItemPlaceholder'] {
- display: none;
}
`;There was a problem hiding this comment.
Thanks. These were leftover from when the Item wasn't a direct child of the slot. I'll clean it up.
There was a problem hiding this comment.
Revisiting this, the proposed suggestion won't really work for our needs and jogged my memory on why the original approach was taken.
There are a couple of issues:
- The
border-radius: inheritfor the button is needed so the focus outline is consistent with the rounded corners - We still need the application of the
border-radiuson the.firstand.lastclasses because theItemGroupstyling doesn't handle hidden elements. (Part of the whole reason for this PR in the first place) - Using
isBorderedandisSeparatedforces everything to visually be represented as an item.
Regarding the last point, it is a problem when in our real-world use case, a contrast checker is also rendered into the panel. This means the contrast checker is not a ToolsPanelItem but appears within the slot which is technically rendered as an ItemGroup. Not forcing isBordered and isSeparated means it can be visually separated.
GIFs demostrating `isBordered` issue
| Suggested Changes | Current Approach |
|---|---|
 |
 |
I've updated the story to include a contrast checker.
There was a problem hiding this comment.
The border-radius: inherit for the button is needed so the focus outline is consistent with the rounded corners
Makes sense, thank you for the explanation!
We still need the application of the border-radius on the .first and .last classes because the ItemGroup styling doesn't handle hidden elements. (Part of the whole reason for this PR in the first place)
Could you show a scenario in which ItemGroup would fail in this scenario? When I came up with the code path above, I tested the Storybook example and everything seems to work fine when showing/hiding toolspanel items.
Using isBordered and isSeparated forces everything to visually be represented as an item.
Regarding the last point, it is a problem when in our real-world use case, a contrast checker is also rendered into the panel. This means the contrast checker is not a ToolsPanelItem but appears within the slot which is technically rendered as an ItemGroup. Not forcing isBordered and isSeparated means it can be visually separated.
Thank you for the explanation! There's an issue, though: ItemGroup would technically expect ContrastChecker to be rendered inside an Item.
The main reason is that by default ItemGroup renders an element with role="list", which itself expects children with role listitem. We could override the role, but then I would argue that we shouldn't use ItemGroup at all, since we would be making use of its 2 major features (list semantics and visual bordered styles).
A potential solution could be to render ItemGroup as a child of ToolsPanel:
ToolsPanel
├── ItemGroup
│ ├── ToolsPanelItem as Item
│ │ ├── [item contents]
│ ├── ToolsPanelItem as Item
│ │ ├── [item contents]
│ ├── ToolsPanelItem as Item
│ │ ├── [item contents]
├── ContrastChecker
although we may probably need to re-apply the grid container styles to ItemGroup. If that didn't work, maybe ItemGroup is not the right fit for this scenario?
There was a problem hiding this comment.
Could you show a scenario in which ItemGroup would fail in this scenario? When I came up with the code path above, I tested the Storybook example and everything seems to work fine when showing/hiding toolspanel items.
The inheritance of the border-radius doesn't work fine because the ItemGroup styles rely on > *:first-of-type > * and > *:last-of-type > * to apply the rounded style border radii. These styles can't take into account whether the element in the item is actually just a non-visible placeholder.
I've updated the story to explicitly disable isRounded on the slot's ItemGroup. As noted previously, we still need styles to handle the border-radius on the .first and .last items. They can be made to inherit from the wrapping ItemGroup (with an extra rule to target the item wrapper), exactly how this styling happens isn't really the issue. We could apply a custom border-radius to the ItemGroup then inherit on the appropriate items or just style those first/last items directly. Either way these first/last classes were needed.
With all the iterations on this, I didn't remove the border color and styling part of the styles as I was distracted by the contrast checker issue when looking at the implications for #34027.
There's an issue, though: ItemGroup would technically expect ContrastChecker to be rendered inside an Item.
This is a fair point and is relevant to #34027. Either way, we still need the classes this PR provides and I don't think should be a blocker here.
I'll remove the contrast checker from the example here so as to avoid future confusion.
A potential solution could be to render ItemGroup as a child of ToolsPanel:
With a view towards #34027, this is easier said than done. Any block or plugin could add new color options to be included within the Inspector Controls color panel. This means the fills that render in the slot come from different places and may contain a mix of color options, contrast checkers or any other element they wish to add into the panel (help text etc).
Moving the contrast checkers to the bottom of the panel was done via CSS ordering in #34027.
I don't believe we can programmatically control the fills, especially when we don't know exactly what we'll be dealing with, so to make the ItemGroup color options only and an inner element of the ToolsPanel we'd likely need to create one or two new slots and nest those inside an already nested slot in the block editor. It makes the color panel a special case with the Inspector Controls group slots and probably more difficult for consumers.
Given that, I'd be inclined in #34027 to no longer use the ItemGroup at all.
If that didn't work, maybe ItemGroup is not the right fit for this scenario
Agreed. This is more a hangover of the color panel being updated without consideration or knowledge that there was a PR open for review to move it to using the ToolsPanel.
I'll update #34027 to remove the use of ItemGroup for the colors panel.
That is obviously separate to this PR so I think this should be ok to land. Since the decision to adopt the new first/last classname props was accepted we've only been iterating on the storybook example.
There was a problem hiding this comment.
Sorry for the delayed response (has a couple of setbacks this week)!
I agree with all of your points — the style rules (and semantics requirements) related to ItemGroup are very strict, and everything is made even more complicated by the fact that tool panel items can be added via slot-fills.
In the light of our findings I think that the best way forward, at least for now, is to avoid using ItemGroup in combination with ToolsPanel — both in #34027 and in this PR (you may even, at this point, re-introduce the contrast checker).
We will also have a look at how to improve the ItemGroup component (of course separately from this PR), in order to make it more flexible/adaptable to different situations (both in terms of styles and in terms of the list semantics).
That is very weird, tests are passing locally for me as well. Maybe this PR needs to be rebased to get the latest version of all deps? |
a373122 to
62c3769
Compare
After rebasing, the emotion styles were different locally as well. The snapshot was updated in 62c3769 and unit tests are passing. |
That's great news! Sorry if this review is taking longer than expected, but this PR is actually of great value to understand any potential limitations that we have with the current set of components and the way each component interacts with each other (e.g. |
It will take as long as it needs 🙂
The catch, in this case, is twofold. First, we are using the ToolsPanel as an ItemGroup which blurs the lines. Second, we aren't controlling what gets added to the ToolsPanel/ItemGroup. So you are right we do have a limitation there but I don't think that is the fault of either component. The likely cleanest approach is to create additional nested slots so we can in fact control (or set expectations for) what is rendered internally to the ToolsPanel. One slot for the color controls, another for anything else. Those fills are then rendered appropriately (within ItemGroup or not) into the color panel slot which itself is within the Inspector Controls slot and so on. As always thanks for the continued effort and attention to detail 👍 |
Very true. In any case, it served as a very good stress test for
Definitely not a fault, but in my opinion a good indication for which direction we need to look at when developing our components further.
That is definitely an idea, but for now I think we can keep it simple (i.e. let's not use |
966c29b to
5fe3da5
Compare
|
I've rebased this PR to resolve conflicts after the merge of the ToolsPanel memorization updates. The unit tests are passing still and storybook examples are all functioning as expected. Once the e2es pass, I'll merge this one. |
|
Just chiming in to say thank you for making structural improvements to the itemgroup. The component is important both for global styles, and for toolspanels going forward, allowing us to much more carefully manage prominence of individual tool controls, while still making them available. The technical implementation is in good hands here, and from a recent implementation challenge, any flexibility in how these are rendered will be welcome 👌 |
|
Related:
Description
The switch of the Color block support panel to the
ToolsPanelrequires rendering the color controls as the new color dropdowns within anItemGroup. To maintain the order of controls when some are injected via SlotFill, theToolsPanelwill render placeholder elements that are hidden. These placeholders interfere with the normalItemGroupstyling which relies on CSS pseudo-selectors e.g.:last-child.To achieve the same result we need to be able to target the first and last displayed
ToolsPanelItems. This PR aims to achieve that by applyingfirstandlastCSS classes to the appropriateToolsPanelItem.How has this been tested?
A simple unit test is included with these changes.
Manually:
ToolsPanelpanels in the sidebar and ensure the correct items get the new classesblock.jsonfile. Test various options like mixing optional and default controls etc.Example ad hoc control for group block
Note this control isn't meant to be fully functional.
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).