Set a default icon margin for buttons with icon and text. #12901
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
gziolo
added
[Type] Bug
|
This seems good to me, and in my testing doesn't break anything anywhere else. Nice work! @gziolo can you sanity check the code changes? It seems good to me on the face of it. 👍 👍 |
|
Thanks. Please do feel free to push the green button (I don't have merge powers). |
afercia
force-pushed
the
update/icon-button-icon-text-margin
branch
from
4ad4b32
to
abd4982
Compare
|
Everything is green and it's already milestoned for 4.9. Merging. |
jasmussen
pushed a commit
that referenced
this pull request
Feb 8, 2019
In #12901, a small margin was introduced to the IconButton component when text was present. This caused an issue with block icons, scaling them down when they shouldn't be. A 20x20px icon should show as 20x20, and a 24x24px icon should show as 24x24px. The additional margin was applied even when the IconButton didn't actually have text. It simply needed `children`, and in the case of the Switcher, it has a dropdown arrow. Combined with the fixed width of the switcher, that meant a 24x24 icon would be rendered as 20x24. This PR adds a length check so if your IconButton includes more than 2 letters, it's considered to have a text label. This fixes the issue. But as you can tell, maybe it's not the best way to check whether there's text or not. Suggestions most welcome.
jasmussen
pushed a commit
that referenced
this pull request
Feb 12, 2019
In #12901, a small margin was introduced to the IconButton component when text was present. This caused an issue with block icons, scaling them down when they shouldn't be. A 20x20px icon should show as 20x20, and a 24x24px icon should show as 24x24px. The additional margin was applied even when the IconButton didn't actually have text. It simply needed `children`, and in the case of the Switcher, it has a dropdown arrow. Combined with the fixed width of the switcher, that meant a 24x24 icon would be rendered as 20x24. This PR adds a length check so if your IconButton includes more than 2 letters, it's considered to have a text label. This fixes the issue. But as you can tell, maybe it's not the best way to check whether there's text or not. Suggestions most welcome.
gziolo
pushed a commit
that referenced
this pull request
Feb 13, 2019
* Fix icon size regression in Switcher In #12901, a small margin was introduced to the IconButton component when text was present. This caused an issue with block icons, scaling them down when they shouldn't be. A 20x20px icon should show as 20x20, and a 24x24px icon should show as 24x24px. The additional margin was applied even when the IconButton didn't actually have text. It simply needed `children`, and in the case of the Switcher, it has a dropdown arrow. Combined with the fixed width of the switcher, that meant a 24x24 icon would be rendered as 20x24. This PR adds a length check so if your IconButton includes more than 2 letters, it's considered to have a text label. This fixes the issue. But as you can tell, maybe it's not the best way to check whether there's text or not. Suggestions most welcome. * Address feedback. Props @aduth for essentially writing this PR.
youknowriad
pushed a commit
that referenced
this pull request
Mar 6, 2019
youknowriad
pushed a commit
that referenced
this pull request
Mar 6, 2019
* Fix icon size regression in Switcher In #12901, a small margin was introduced to the IconButton component when text was present. This caused an issue with block icons, scaling them down when they shouldn't be. A 20x20px icon should show as 20x20, and a 24x24px icon should show as 24x24px. The additional margin was applied even when the IconButton didn't actually have text. It simply needed `children`, and in the case of the Switcher, it has a dropdown arrow. Combined with the fixed width of the switcher, that meant a 24x24 icon would be rendered as 20x24. This PR adds a length check so if your IconButton includes more than 2 letters, it's considered to have a text label. This fixes the issue. But as you can tell, maybe it's not the best way to check whether there's text or not. Suggestions most welcome. * Address feedback. Props @aduth for essentially writing this PR.
youknowriad
pushed a commit
that referenced
this pull request
Mar 6, 2019
youknowriad
pushed a commit
that referenced
this pull request
Mar 6, 2019
* Fix icon size regression in Switcher In #12901, a small margin was introduced to the IconButton component when text was present. This caused an issue with block icons, scaling them down when they shouldn't be. A 20x20px icon should show as 20x20, and a 24x24px icon should show as 24x24px. The additional margin was applied even when the IconButton didn't actually have text. It simply needed `children`, and in the case of the Switcher, it has a dropdown arrow. Combined with the fixed width of the switcher, that meant a 24x24 icon would be rendered as 20x24. This PR adds a length check so if your IconButton includes more than 2 letters, it's considered to have a text label. This fixes the issue. But as you can tell, maybe it's not the best way to check whether there's text or not. Suggestions most welcome. * Address feedback. Props @aduth for essentially writing this PR.
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.
Description
This small PR seeks to improve the IconButton component by making sure there's some spacing between the button's icon and text.
:not:only-childas it doesn't work with text nodes (it was also missing the parenthesis)Notes:
4pxmargin but I've seen the other cases mentioned above use a5pxvalue, not sure which is the one to use/Cc @jasmussen
Example screenshot before:
After:
Fixes #12900