Icons: Remove width / height attributes from svg element #43747
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
[Type] Bug
|
Size Change: -67 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
t-hamano
changed the title
|
Hi, thank you for the PR! I'm actually not sure this is a good idea, and on the contrary it's my main instinct that all of our icons should have preset width and heights baked in. For two reasons:
The 2nd issue is the most important one, especially in the site editor where the CSS can take a bit longer to load, causing a flash of a big icon. We recently fixed this for the spinner. You can see the flash of a big spinner here: #43226 Obviously the What do you think? |
|
cc @mirka |
That makes sense, since icons from the Icons library might be used directly without going through the component. I went back through the history of the Icons package and couldn't find any clear argument or reason why almost all icons don't have Is your suggestion to give |
mirka
reviewed
Sep 1, 2022
There was a problem hiding this comment.
I think it's fine to assume that the SVGs in our icon library will always be used via the Icon component.
The Icon component adds width/height attributes to the SVG based on the size prop, so in reality the SVGs will have a set width/height at render time, regardless of whether the original SVG code includes it. So if we can assume that icon SVGs will be used via Icon, we don't have to worry about the icon files themselves having a width/height attribute.
The main bug here is that the width/height set by the Icon component will be overwritten again by the icon's built-in width/height 😄
gutenberg/packages/components/src/icon/index.tsx
Lines 85 to 87 in 9581694
Reverse this order and we're good 👍
Thanks for identifying the bug! |
|
The issue of the Button |
t-hamano
force-pushed
the
fix/icons-width-height-attr
branch
from
929f326 to
4722a0c
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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: #43745Related: #40315
What?
This PR removes the
width/heightattributes from the svg element in the@wordpress/iconsicon.Why?
As tested in #43745, the
iconSizeprop is not applied when an icon withwidth/heightattributes is specified in theButtoncomponent.@edit: This issue is fixed by #43821.
We could not find a clear reason why the
Iconscomponent could resize these icons correctly, but at least the svg icons themselves don't needwidth/height, and removing these attributes gave the expected results.How?
Simply removed attributes.
Testing Instructions