ToolsPanel: Remove hardcoded classnames #35415
Conversation
aaronrobertshaw
added
[Feature] UI Components
aaronrobertshaw
requested review from
ajitbohra and
chrisvanpatten
as code owners
aaronrobertshaw
force-pushed
the
remove/toolspanel-hardcoded-classnames
branch
from
65de7b8
to
0c324a8
Compare
|
This tested well for me in Storybook and in local site cover block. The emotion css stuff is new to me though, so will leave it to someone with more experience in that to sign it off. |
There was a problem hiding this comment.
Thank you for working on this, @aaronrobertshaw !
I left a couple of comments. I would also love to hear @sarayourfriend 's opinion on this PR and the solutions we've adopting. I'm also cc'ing @mirka for visibility.
| /* Required to meet specificity requirements to ensure zero margin */ | ||
| && { |
There was a problem hiding this comment.
This specificity hack shouldn't be necessary — I tried removing it and the spacing still looks correct (at least in Storybook).
I then noticed that, in the markup, we're using a h2 and that's we're adding flexbox styles manually. And so, I played around and leveraged the HStack and Heading components to reduce the amount of custom CSS needed in a component (similarly to what we're going to do by leveraging Grid in a future PR).
Using Heading this way can also help us improve the overall consistency of the typography system (see related #35464)
This is what I came up with
diff --git a/packages/components/src/tools-panel/styles.ts b/packages/components/src/tools-panel/styles.ts
index 5135d2287e..6de4d21a5b 100644
--- a/packages/components/src/tools-panel/styles.ts
+++ b/packages/components/src/tools-panel/styles.ts
@@ -57,18 +57,7 @@ export const ToolsPanelHiddenInnerWrapper = css`
`;
export const ToolsPanelHeader = css`
- align-items: center;
- display: flex;
- font-size: inherit;
- font-weight: 500;
${ toolsPanelGrid.item.fullWidth }
- justify-content: space-between;
- line-height: normal;
-
- /* Required to meet specificity requirements to ensure zero margin */
- && {
- margin: 0;
- }
/* Tweak dropdown menu and toggle button for better alignment */
> div {
@@ -86,6 +75,12 @@ export const ToolsPanelHeader = css`
}
`;
+export const ToolsPanelHeading = css`
+ font-size: inherit;
+ font-weight: 500;
+ line-height: normal;
+`;
+
export const ToolsPanelItem = css`
${ toolsPanelGrid.item.fullWidth }
diff --git a/packages/components/src/tools-panel/tools-panel-header/component.tsx b/packages/components/src/tools-panel/tools-panel-header/component.tsx
index 3204615434..0373f6adb7 100644
--- a/packages/components/src/tools-panel/tools-panel-header/component.tsx
+++ b/packages/components/src/tools-panel/tools-panel-header/component.tsx
@@ -16,6 +16,8 @@ import { __, sprintf } from '@wordpress/i18n';
import DropdownMenu from '../../dropdown-menu';
import MenuGroup from '../../menu-group';
import MenuItem from '../../menu-item';
+import { Heading } from '../../heading';
+import { HStack } from '../../h-stack';
import { useToolsPanelHeader } from './hook';
import { contextConnect, WordPressComponentProps } from '../../ui/context';
import type {
@@ -122,6 +124,7 @@ const ToolsPanelHeader = (
menuItems,
resetAll,
toggleItem,
+ headingClassName,
...headerProps
} = useToolsPanelHeader( props );
@@ -133,8 +136,10 @@ const ToolsPanelHeader = (
const optionalItems = Object.entries( menuItems?.optional || {} );
return (
- <h2 { ...headerProps } ref={ forwardedRef }>
- { labelText }
+ <HStack { ...headerProps } ref={ forwardedRef }>
+ <Heading level={ 2 } className={ headingClassName }>
+ { labelText }
+ </Heading>
{ hasMenuItems && (
<DropdownMenu
icon={ moreVertical }
@@ -168,7 +173,7 @@ const ToolsPanelHeader = (
) }
</DropdownMenu>
) }
- </h2>
+ </HStack>
);
};
diff --git a/packages/components/src/tools-panel/tools-panel-header/hook.ts b/packages/components/src/tools-panel/tools-panel-header/hook.ts
index f31e8cb637..26a2ebca57 100644
--- a/packages/components/src/tools-panel/tools-panel-header/hook.ts
+++ b/packages/components/src/tools-panel/tools-panel-header/hook.ts
@@ -25,6 +25,11 @@ export function useToolsPanelHeader(
return cx( styles.ToolsPanelHeader, className );
}, [ className ] );
+ const headingClassName = useMemo(
+ () => cx( styles.ToolsPanelHeading ),
+ []
+ );
+
const dropdownMenuClassName = useMemo( () => {
return cx( styles.DropdownMenu );
}, [] );
@@ -33,9 +38,10 @@ export function useToolsPanelHeader(
return {
...otherProps,
- dropdownMenuClassName,
hasMenuItems,
menuItems,
className: classes,
+ dropdownMenuClassName,
+ headingClassName,
};
}There was a problem hiding this comment.
Thanks @ciampo for going the extra mile yet again in your reviews, it's appreciated 👍
The increased specificity was required for use outside of the storybook. Previous tweaks to the heading's styles from within the block editor to overcome its own heading styles were requested to be removed and that the component should ensure the zero margin itself, hence the rule as it stands now.
The use of the h2 is one of the last remnants of this component having evolved from a basic clone of the existing panel.
I'll roll your proposed changes into this PR tomorrow when I'll have more time to test it all with real-world use cases such as block supports.
There was a problem hiding this comment.
Thanks @ciampo for going the extra mile yet again in your reviews, it's appreciated 👍
It's my pleasure — it's important that we share knowledge as much as possible, especially around the components package and its specific requirements.
The increased specificity was required for use outside of the storybook.
Thank you for the context. I suspected there must have been a use case for it 😅
I'll roll your proposed changes into this PR tomorrow when I'll have more time to test it all with real-world use cases such as block supports.
No rush as always. I hope these changes will still hold up well in real-world use 🤞
There was a problem hiding this comment.
The switch to using HStack and Heading is a nice step forward. There were two catches with it that I found while testing.
- The hack to enforce
margin-bottom: 0is still required to overcome editor styles. - The
HStack'sdivconflicted with the styles aiming to re-establish a grid layout when slots wrap the fills in adiv
As I continue to work on switching the panel to use the Grid component to handle layout, I'll see if that second issue can't be mitigated.
There was a problem hiding this comment.
Thank you for the explanation!
Regarding your second point, I think a better way to write that workardound could be to use the ToolsPanelHeader styles object as a selector
Example:
diff --git a/packages/components/src/tools-panel/styles.ts b/packages/components/src/tools-panel/styles.ts
index 39fd6e4e74..270dca54e8 100644
--- a/packages/components/src/tools-panel/styles.ts
+++ b/packages/components/src/tools-panel/styles.ts
@@ -38,20 +38,31 @@ export const ToolsPanel = css`
padding: ${ space( 4 ) };
`;
+export const ToolsPanelHeading = css`
+ font-size: inherit;
+ font-weight: 500;
+ line-height: normal;
+
+ /* Required to meet specificity requirements to ensure zero margin */
+ && {
+ margin: 0;
+ }
+`;
+
/**
* Items injected into a ToolsPanel via a virtual bubbling slot will require
* an inner dom element to be injected. The following rule allows for the
* CSS grid display to be re-established.
*/
export const ToolsPanelWithInnerWrapper = css`
- > div:not( :first-of-type ) {
+ > div:not( ${ ToolsPanelHeading } ) {
${ toolsPanelGrid.container }
${ toolsPanelGrid.item.fullWidth }
}
`;
export const ToolsPanelHiddenInnerWrapper = css`
- > div:not( :first-of-type ) {
+ > div:not( ${ ToolsPanelHeading } ) {
display: none;
}
`;
@@ -81,17 +92,6 @@ export const ToolsPanelHeader = css`
}
`;
-export const ToolsPanelHeading = css`
- font-size: inherit;
- font-weight: 500;
- line-height: normal;
-
- /* Required to meet specificity requirements to ensure zero margin */
- && {
- margin: 0;
- }
-`;
-
export const ToolsPanelItem = css`
${ toolsPanelGrid.item.fullWidth }
There was a problem hiding this comment.
@ciampo I don't think it's possible to use the serialized styles object as a selector is it? I thought it was just styled components that could be (but I'll be happily surprised if I'm wrong!)
There was a problem hiding this comment.
The suggested change doesn't work for me. No errors are thrown however those styles are omitted.
There might be a way to achieve a valid syntax here that I'm missing. I'm not sure it offers much at this stage though.
There was a problem hiding this comment.
My bad. I tested it by checking it the WithSlotFillItems Storybook example thinking that it would cover this case, but I was wrong.
I thought it was just styled components that could be (but I'll be happily surprised if I'm wrong!)
Me too, but then testing on the wrong Storybook example tricked me into thinking it would work 😅
ciampo
moved this from In progress (owned) ⏳
to In progress ⏳ (un-owned)
in WordPress Components
The most common use of the base control within the ToolsPanel has it one level deeper. The cleanest option appears to be blanket removal of the margin when within a ToolsPanel, if the component using BaseControl is more complex it can override that style as needed.
aaronrobertshaw
force-pushed
the
remove/toolspanel-hardcoded-classnames
branch
from
2a6ecbe
to
d684a45
Compare
|
This has been rebased after the plus icon changes for the panel menu were merged. All the requested changes have been made. Assuming that the tweak to enforce no bottom margin on the header is acceptable, I think this should be close now. |
|
Size Change: -418 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
I have a follow-up PR to this one switching the |
There was a problem hiding this comment.
Thank you @aaronrobertshaw!
I left a couple more comments, but feel free to merge once they are addressed 🚀
I have a follow-up PR to this one switching the ToolsPanel to leverage the Grid component removing the reliance on the single-column class
Excellent, thank you! Just FYI I may not be able to have a look at it today (but I'll try tomorrow) — thank you!
Another nice follow-up to this PR would be to type the DropdownMenu component, in order to get rid of the last hardcoded classnames — although I understand if can't look at it now, since other work on ToolsPanel and Typography tools may have higher priority.
There's no hurry 🙂 The switch to using the
It would be a nice follow up. Unfortunately, it will not be something that I can work on. The typography tools have my immediate focus and following that I'll need to return to a greater focus on design tooling for blocks. Happy to help out with reviews etc if someone else would like to pick up that conversion. |
No problems at all! I already really appreciate the help you've been giving with |
ciampo
added
[Type] Code Quality
Fixes: #35058
Description
This PR removes the use of hardcoded CSS classnames from the
ToolsPanelstyles. The one exception is thesingle-columnclass as this will be removed an a separate PR adopting the Grid component to handle layout for the `ToolsPanel.How has this been tested?
Manually.
Testing Instructions
npm run storybook:devToolsPanelstory and confirm it still displays and functions correctlyScreenshots
Types of changes
Enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).