-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(button): Support icon in button #1281
Conversation
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
Codecov Report
@@ Coverage Diff @@
## master #1281 +/- ##
======================================
Coverage 99.9% 99.9%
======================================
Files 69 69
Lines 3314 3314
Branches 409 409
======================================
Hits 3311 3311
Misses 3 3 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we're documenting this, it probably makes sense to also include it in the demo page.
(I think we initially discussed possibly not including a demo of it, but based on a discussion I just had with Lynn, we feel like we should include one.)
025c0bb
to
745d845
Compare
demos/button.html
Outdated
<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Roboto+Mono"> | ||
<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Roboto:300,400,500"> | ||
<link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons"> | ||
<script src="/assets/demo-styles.css.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this line was moved in this PR, but it will no longer exist the next time you merge with master anyway. The styles have been split out, so now the ones for this demo are in button.scss
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rebase this PR on #1285 .
To address your question though, this line introduced a funny bug. We defined mdc-button__icon
in button.scss
, which is sourced as part of material-component-web.css
in demo.scss
. However, we added "Material+Icons" stylesheet below that to introduce material-icon
. The latter introduced a font-size: 24px
that override what we provided in demo.scss
(button.scss
) and that's the reason why I moved it to the bottom because we definitely don't want a general library to override our own styles.
It shouldn't be a concern any longer after I rebased it.
demos/button.html
Outdated
@@ -147,6 +147,10 @@ <h1 class="mdc-typography--display2">Ripple Enabled</h1> | |||
<a href="javascript:void(0)" class="mdc-button"> | |||
Link | |||
</a> | |||
<button class="mdc-button"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wanted to keep link last since it's the only one that doesn't get disabled with the checkbox at the top of the demo page, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't hold a strong opinion here, but this is a reasonable ask since that looks cleaner. Will move it.
packages/mdc-button/_mixins.scss
Outdated
height: 16px; | ||
margin-right: 8px; | ||
font-size: 16px; | ||
vertical-align: middle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These icons don't actually look like they're vertically centered; their tops align with the text, but they look like they're extending below the baseline, which is going to look awkward at least with all-caps text like our buttons typically have. I'm not sure what the design goal is here though - technically right now I think our button text vertical placement is centered disregarding anything hanging below the baseline.
Also, I'm wondering how this will interact with dense button, or is it not intended to ever be combined with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align with center is a reasonable assumption. Let me see if I could address this. The problem here is the button text is 14px (and 13px for dense button). However, the targeted icon side is 16px. I was not sure about the intention and may check with design tomorrow in the sign off.
745d845
to
805f2e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I also verified in Edge by merging #1302 into this locally.
2c85a4f
to
7ebee64
Compare
Thanks for checking it! I will wait a bit for CI to finish... |
Related to #991
Codepen Demo
Dev server: 1281-dot-mdc-web-dev.xxxx.com