Button: Remove unnecessary margin from dashicon #51395
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
|
Size Change: -30 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in bbd89ed. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5297188139
|
t-hamano
force-pushed
the
button/dashicon-margin
branch
from
a8da456
to
833c720
Compare
t-hamano
commented
Jun 10, 2023
Comment on lines
293
to
296
| .dashicon { | ||
| display: inline-block; | ||
| flex: 0 0 auto; | ||
| margin-left: 2px; | ||
| margin-right: 2px; | ||
| } |
There was a problem hiding this comment.
This left/right margin exists to widen the default size of the dash icon, 20px, to 24px.
As this comment suggests, I would ideally like to remove these margins as well, in order to solve the problem more upstream when the dashicon is used in the DropdownMenu component, but so far I have not found a satisfactory approach.
One approach I thought of is to use padding instead of margin to widen the size and center the dashicon font of the pseudo-element inside.
.dashicon {
display: inline-flex;
justify-content: center;
align-items: center;
padding: 2px;
box-sizing: content-box;
}
There was a problem hiding this comment.
Yes, padding might be a little less fragile than margin.
mirka
force-pushed
the
button/dashicon-margin
branch
from
5ef483e
to
8b3db8d
Compare
mirka
approved these changes
Jun 16, 2023
There was a problem hiding this comment.
Looks good! 🚀
I hope you don't mind that I push a local visual regression test to this PR. I can split it out to a separate PR if you prefer.
Vizreg results
We can see that the changes in this PR only affect Buttons where the text is specified via children (not the text prop), and also that the iconPosition prop has no affect in this case 😅 so we're good.
Before
After
Comment on lines
293
to
296
| .dashicon { | ||
| display: inline-block; | ||
| flex: 0 0 auto; | ||
| margin-left: 2px; | ||
| margin-right: 2px; | ||
| } |
There was a problem hiding this comment.
Yes, padding might be a little less fragile than margin.
|
Also I'm not sure if you're already aware, but we can test dashicons in Storybook now. The "WordPress" option in the global CSS toolbar addon will load the dashicons stylesheet/font. Mentioning it just in case 🙂 
|
t-hamano
force-pushed
the
button/dashicon-margin
branch
from
8b3db8d
to
0b58a1b
Compare
|
Thanks for the review and for adding the visual regression test 🙏 I have added the approach suggested in this comment and performed a visleg test locally and got the same expected results. I would like to merge when all CIs have passed 👍 |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* Add vizreg test * Button: Remove unnecessary margin from dashicon * Update changelog * Revert some changes * Layout with padding instead of margin --------- Co-authored-by: Lena Morita <[email protected]>
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.
What?
This PR removes the extra space that is added between the icon and the text when the dashicon is used in a
Buttoncomponent.Why?
In #50277, the gap is now used for spacing between icon and text. However, for dashicon,
margin-rightwas still used for the spading between the icon and the text.How?
Removed unnecessary margin style. At the same time, I also removed unnecessary styles (
display: inline-block; flex: 0 0 auto;) from the dashicon since it's a child of flex.Testing Instructions
Update the Edit component (
packages/block-library/src/code/edit.js) of the code block as follows:Confirm that the width of the
Buttoncomponent with theIconcomponent matches the width of theButtoncompoent with the dashicon.Screenshots or screencast