This repository has been archived by the owner on Jan 13, 2025. It is now read-only.
fix(button): Suppress whitespace between icon and text label #2449
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
aprigogin
reviewed
Mar 23, 2018
| @@ -95,7 +95,7 @@ | |||
| @include mdc-ripple-radius-bounded; | |||
| @include mdc-button-horizontal-padding($mdc-button-horizontal-padding); | |||
|
|
|||
| display: inline-block; | |||
| display: inline-flex; | |||
There was a problem hiding this comment.
I think you might want to change
text-align: center; to justify-content: center;
aprigogin
reviewed
Mar 23, 2018
packages/mdc-button/_mixins.scss
Outdated
| fill: currentColor; | ||
| vertical-align: middle; | ||
| align-self: center; |
There was a problem hiding this comment.
maybe align-items: center on button_base?
Codecov Report
@@ Coverage Diff @@
## master #2449 +/- ##
=======================================
Coverage 98.91% 98.91%
=======================================
Files 104 104
Lines 4234 4234
Branches 534 534
=======================================
Hits 4188 4188
Misses 46 46Continue to review full report at Codecov.
|
aprigogin
approved these changes
Mar 26, 2018
williamernest
pushed a commit
that referenced
this pull request
Mar 27, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Opening this for potential discussion. This is a follow-on to #2420 / #2400.
This provides a solution to the problem where whitespace between the icon and text label (which is common since the icon tends to be on its own line in HTML source) causes the spacing between icon and text to be larger than intended by design.
This fixes it by changing button's display property from
inline-blocktoinline-flex. This doesn't have any effects on usage of button in terms of its flow among other external content.However, it does have one implication for the button's text label content: if the text label includes any child elements, then the entire label needs to be nested in a child element to avoid inline-flex collapsing the space between text nodes and/or child elements. This is illustrated in several updates to demo pages in this PR to keep them consistent with before.
This ultimately comes down to how common/supported we expect the use case of child elements within text labels to be. We could add a caveat note to the readme if we expect users to run across it.