-
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
fix(toolbar): Fix icon padding for ripples, and vertical alignment in FF/IE/Edge #2138
Conversation
demos/toolbar/index.html
Outdated
<a href="#" class="material-icons mdc-toolbar__icon" aria-label="Download" alt="Download">file_download</a> | ||
<a href="#" class="material-icons mdc-toolbar__icon" aria-label="Print this page" alt="Print this page">print</a> | ||
<a href="#" class="material-icons mdc-toolbar__icon" aria-label="Bookmark this page" alt="Bookmark this page">bookmark</a> | ||
<a href="#" class="material-icons mdc-toolbar__icon mdc-ripple-surface demo-toolbar-surface" data-mdc-ripple-is-unbounded aria-label="Download" alt="Download">file_download</a> |
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.
Should we call @include mdc-states(text-primary-on-primary)
from within the CSS for mdc-toolbar__icon
? And then remove this mdc-ripple-surface
from the demo?
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 can probably add ripple styles to mdc-toolbar__icon
if it's safe to assume all toolbar icons are intended to be interactable (which I think is the case judging by the spec).
If I do that, I should probably also add logic to toolbar's JS to instantiate ripples for its icons.
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 think we should do that.
@williamernest was cool enough to find a resolution to the snag I ran into with flexible toolbar, and I've committed the rest of my stuff on top of that. Will's now hammering out one remaining issue but then I think this is ready for review again. |
b831173
to
8dff673
Compare
Codecov Report
@@ Coverage Diff @@
## master #2138 +/- ##
==========================================
- Coverage 99.21% 99.04% -0.18%
==========================================
Files 99 99
Lines 3967 3976 +9
Branches 510 510
==========================================
+ Hits 3936 3938 +2
- Misses 31 38 +7
Continue to review full report at Codecov.
|
align-items
tocenter
for toolbar section element to fix vertical alignment in IE/Edge and also mitigate the adjustments to vertical padding in this PRThis should not cause a visible difference in icon padding/arrangement (other than vertically aligning it properly in IE/Edge).
Fixes #1067 (finally)
Also fixes #1296 related to vertical alignment (I re-checked and indeed it's not just IE/Edge, it affects FF too, huh.)