-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(toolbar): Fix toolbar padding on desktop and mobile #1327
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1327 +/- ##
======================================
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.
Good catch finding these issues. One nit, otherwise LGTM based on our discussion/review earlier today.
packages/mdc-toolbar/_variables.scss
Outdated
$mdc-toolbar-element-vertical-padding: 16px; | ||
$mdc-toolbar-element-horizontal-padding-desktop: 24px; | ||
$mdc-toolbar-element-horizontal-padding-mobile: 16px; | ||
$mdc-toolbar-title-margin-to-icon-menu-desktop: 8px; |
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.
Nit: would these names read better as title-margin-to-menu-icon
(rather than icon-menu
)?
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 seems this won't introduce confusion with our modifier class name after offline discussion, so I am going to change it this way to make it read better.
Also, this should be fix(toolbar), not fix(button), I think, when we merge. |
Thanks for catching the commit message error! |
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. Also probably change "button" to "toolbar" in the commit message itself, forgot to mention that before.
According to guideline the padding seems not right:
https://material.io/guidelines/layout/metrics-keylines.html#metrics-keylines-keylines-spacing
After the changes, it became:
Desktop:
Mobile: