-
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(snackbar): Drop mdc-button from snackbar's dependency #1292
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1292 +/- ##
======================================
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.
|
0724b1b
to
95e9388
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.
This mostly looks good but I noticed one thing that seemed odd.
@include mdc-theme-prop(color, secondary); | ||
|
||
@include mdc-theme-dark(".mdc-snackbar") { | ||
@include mdc-theme-prop(color, primary); | ||
} | ||
|
||
@include mdc-rtl-reflexive-box(margin, right, -16px, ".mdc-snackbar"); |
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.
Why was this line removed? This effectively shifts the action button to the left 16px.
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.
This line used to be the mechanism to reverse padding
introduce by button, since now there is no longer padding: 0 16px
, we also could safely remove the margin-direction: -16px
here.
Could you take a look at the deployed dev server and see if this style looks correct to you?
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.
It appears like the spacing isn't the same between the start aligned LTR and RTL contexts. It looks like the leading margin the LRT and the trailing margin in RTL are different. Attaching screen shots
Good catch, seems like Chrome add some padding by default to button. My overlook indeed! Thanks @amsheehan |
BREAKING CHANGE: Removed the dependency of mdc-button from DOM structure of snackbar.
95e9388
to
6548a9b
Compare
Sorry Ken to dismiss your review. Feel relax to rest at home :)
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 @amsheehan's observation may have coincided with my own, and it looks like it's resolved now. Thanks both of you!
BREAKING CHANGE: Removed the dependency of mdc-button from DOM structure of snackbar.
Dev server: 1292-dot-mdc-web-dev.xxxx.com
Background
The rationale behind dropping button from snackbar is
action-button
is not technically aMDC button
from the design perspective.So to prevent future regression, we remove the dependency of button from snackbar by directly styling
action button
. The changes increase CSS file by 10 lines, but the actual size remained same 10KB.