DropdownMenu: fix icon style when dashicon is used #43574
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Feature] Rich Text
t-hamano
commented
Aug 24, 2022
| } | ||
|
|
||
| // Formatting buttons | ||
| > svg { | ||
| > svg, | ||
| > span.dashicon { |
There was a problem hiding this comment.
Increased specificity as span.dashicon instead of dashicon to override some of the default styles for the button component.
|
Size Change: -13 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
mirka
reviewed
Aug 30, 2022
Comment on lines
50
to
56
| margin: 0 8px 0 0; | ||
| } | ||
|
|
||
| > span.dashicon { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; |
There was a problem hiding this comment.
Just checking — have we considered whether these margin and flex styles should be in the Button component itself? I couldn't really check because of #43700, but it seems like that might be the case.
There was a problem hiding this comment.
Indeed, I should have considered that first.
Dashicons are displayed at 20x20 and SVG icons at 24x24. And in #27461, it seems that Dashicon was also considered to be displayed at 24x24, but finally settled at 20x20.
However, the dash icon must be displayed at 24x24 in DropdownMenu in order for .is-active style to be displayed at the same size.
This is why I did this, but perhaps it should be done in the button component.
In the current button component, the dashicon remains 20x20px and appears to have been adjusted to balance with the SVG icon.
Should the Dashicon be 24x24 in the button component as well?
Code
/**
* WordPress dependencies
*/
import { useBlockProps } from '@wordpress/block-editor';
import { helpFilled } from '@wordpress/icons';
import { Button } from '@wordpress/components';
export default function CodeEdit() {
const blockProps = useBlockProps();
return (
<div { ...blockProps }>
<p>Dashicon</p>
<Button
icon="editor-help"
variant="primary"
style={ { marginRight: 8 } }
>
Push Me !
</Button>
<Button icon="editor-help" variant="primary" />
<p>SVG Icon</p>
<Button
icon={ helpFilled }
variant="primary"
style={ { marginRight: 8 } }
>
Push Me !
</Button>
<Button icon={ helpFilled } variant="primary" />
</div>
);
}There was a problem hiding this comment.
Thanks for digging!
I don't have an opinion on whether dashicons in Button should be 20 or 24px. If we do want it to be 24px, we can do #43724. If we want to keep it at 20px, I think we should wrap each Button Icon in a centering flex container at iconSize width/height so we can maintain a consistent size between dashicons and SVGs. That would be more robust than the hardcoded 2px margin thing that is going on right now.
I also made an issue (#43725) for the deprecation suggested in the PR you mentioned. Though, I imagine we would need a long deprecation period, because it's probably very widely used.
There was a problem hiding this comment.
I have been rethinking what the best way to go about this is.
I think the size of the icon in the Button component should be only defined by the iconSize prop and not explicitly given width and height.
In other words, dashicons should remain at the default 20px, or 24px for SVG icons, and only when the iconSize prop is specified should the size of the element (font-size for dashicons) change according to that value.
(However, as noted in #43724, the iconSize property does not currently support dashicons.)
Therefore, it would make sense that when a dashicon is used in a Button component, it is adjusted to the default 20px size. However, in DropdownMenu, the size of the background color that represents the active state must be consistent with the size of the element, whether it is an svg icon or a dashicon.
Therefore, I have finally decided that this change by PR makes sense, what do you think?
There was a problem hiding this comment.
I'm only skeptical about the margin and flex styles being here (lines 50–56). The margin override is especially hacky. I'm thinking it could be cleaner if we cleaned it up upstream so it doesn't have the hacky side margins to begin with:
gutenberg/packages/components/src/button/style.scss
Lines 309 to 314 in e935c5d
↓
.dashicon {
display: inline-flex;
justify-content: center;
align-items: center;
flex: 0 0 auto;
}(I haven't tested in all Button variants, but something like that.)
There was a problem hiding this comment.
Thanks for the advice!
I was looking for a fix in the upstream button component and found two issues:
- Component: Button text is hidden in certain variations #44043
- Component: Order and spacing issues for text and icon in Button component #44042
I would like to put these two issues as blockers and hold off on moving forward with this PR, as I think these two should be resolved first.
t-hamano
force-pushed
the
fix/dropdownmenu-dashicon
branch
from
fa67bd5
to
3101339
Compare
|
Update: In #51395, the default size of the dashicon is now 24, similar to the Below are the latest screenshots.
|
t-hamano
commented
Jun 24, 2023
Comment on lines
-41
to
-45
| // Formatting buttons | ||
| > svg { | ||
| border-radius: $radius-block-ui; | ||
| width: $button-size-small; | ||
| height: $button-size-small; |
There was a problem hiding this comment.
I don't think border-radius is necessary when the menu item is not active. Also, since the icon is guaranteed to be 24px in size, I don't think width and height are necessary.
|
Flaky tests detected in 3101339. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5364161331
|
mirka
approved these changes
Jun 29, 2023
|
Thanks for the review, @mirka! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #29376, #40138
What?
This PR fixes a problem where the controls property of the
DropdownMenucomponent doesn't apply the correct style when dashicon is specified.Why?
In #29376 and #40138, you can see sample code using
registerFormatTypeandRichTextToolbarButton. These codes add new formatting capabilities to rich text and are added in the same dropdown as the core formatting.Also, the icon property of the
RichTextToolbarButtonis internally passed to theButtoncomponent within theDropdownMenucomponent.This button icon style is applied correctly for SVG icons such as
@wordpress/icons, but not for dashicons that are output as span tags. Therefore, it is not visible that the format is applied (i.e., the icon is active).At the same time, there is a difference in the default style when the SVG icon and the dash icon are used at the same time.
How?
I have added selectors so that styles for SVG icons also apply to dashicons. At the same time,
I have added a style to center it because the dashicon font is displayed as a pseudo-element.I have removed unnecessary styles.Testing Instructions
Screenshots or screencast