feat(top-app-bar): Add prominent style #2349
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2349 +/- ##
=======================================
Coverage 98.85% 98.85%
=======================================
Files 100 100
Lines 4118 4118
Branches 527 527
=======================================
Hits 4071 4071
Misses 47 47Continue to review full report at Codecov.
|
demos/top-app-bar.html
Outdated
| @@ -227,9 +243,13 @@ <h3>Demo Controls</h3> | |||
| appBarEl.classList.remove('mdc-top-app-bar--short-collapsed'); | |||
| appBarEl.classList.remove('mdc-top-app-bar--short-has-action-item'); | |||
| alwaysCollapsedCheckbox.checked = false; | |||
| } else { | |||
There was a problem hiding this comment.
Nit: reverse the if/else so the if is the positive condition
| @@ -238,6 +258,10 @@ <h3>Demo Controls</h3> | |||
| appBar.destroy(); | |||
| appBar = mdc.topAppBar.MDCTopAppBar.attachTo(appBarEl); | |||
| }); | |||
|
|
|||
| prominentCheckbox.addEventListener('change', function() { | |||
| appBarEl.classList[this.checked ? 'add' : 'remove']('mdc-top-app-bar--prominent'); | |||
There was a problem hiding this comment.
I'd point out that we're disabling the prominent checkbox when short is checked, but not vice versa... do we want these to consistently either toggle each other or disable each other? (If we make it so that prominent disables short, we shouldn't need to uncheck prominent in the short checkbox handler since it would need to be unchecked manually first)
There was a problem hiding this comment.
Fixed. I changed it so the prominent disabled the short checkbox and vice versa.
demos/top-app-bar.scss
Outdated
| flex-direction: column; | ||
| margin: 0 20px 20px 20px; | ||
|
|
||
| & > span { |
There was a problem hiding this comment.
We should really avoid element selectors... Could we use something like <h4 class="mdc-typography--subheading2 mdc-typography--adjust-margin for these elements instead? (And maybe use mdc-typography--title on the Demo Controls heading?)
package-lock.json
Outdated
| @@ -2,6 +2,16 @@ | |||
| "requires": true, | |||
| "lockfileVersion": 1, | |||
| "dependencies": { | |||
| "JSONStream": { | |||
There was a problem hiding this comment.
I don't think package-lock needs to be in this PR...
| @@ -130,5 +130,24 @@ | |||
| } | |||
| } | |||
|
|
|||
| .mdc-top-app-bar--prominent { | |||
| .mdc-top-app-bar__row { | |||
| min-height: $mdc-top-app-bar-prominent-row-height; | |||
There was a problem hiding this comment.
Why is this using min-height rather than height? Would something cause this to be even taller? And would we not be eventually interested in this collapsing to one line's height on scroll?
| padding-bottom: $mdc-top-app-bar-prominent-title-bottom-padding; | ||
| } | ||
|
|
||
| .mdc-top-app-bar__action-item { |
There was a problem hiding this comment.
Combine this and the next selector into one block?
| @@ -55,3 +55,21 @@ | |||
| input[type="checkbox"]:disabled + label { | |||
| opacity: .4; | |||
| } | |||
|
|
|||
demos/top-app-bar.scss
Outdated
| display: flex; | ||
| flex: 1 1 auto; | ||
| flex-direction: column; | ||
| margin: 0 20px 20px 20px; |
There was a problem hiding this comment.
Nit: last 20px can be omitted
|
|
||
| .mdc-top-app-bar--prominent { | ||
| .mdc-top-app-bar__row { | ||
| height: $mdc-top-app-bar-prominent-row-height; |
There was a problem hiding this comment.
So, RE my earlier comment RE min-height, I just realized that you're using min-height everywhere that you're setting height for __row, so I guess my question applies to all of them. I imagine in these cases you're intentionally overriding those, so it makes sense for them to match, I'm just not sure offhand why they use min-height.
|
|
||
| .mdc-top-app-bar--prominent { | ||
| .mdc-top-app-bar__row { | ||
| height: $mdc-top-app-bar-prominent-row-height; |
There was a problem hiding this comment.
Also, shouldn't this line not even be necessary if you decrease the specificity of .mdc-top-app-bar .mdc-top-app-bar__row to just the latter class, further above in this mixin? I don't think those first 3 selectors should need 2 classes each, since they don't have 2 classes each in the default styles.
There was a problem hiding this comment.
The first three selectors do have 2 classes each in the default styles.
…erial-components/material-components-web into feat/top-app-bar/add-prominent-style
| } | ||
|
|
||
| .mdc-top-app-bar--short { | ||
| transition: width 200ms $mdc-animation-standard-curve-timing-function; | ||
|
|
||
| .mdc-top-app-bar__row { | ||
| min-height: $mdc-top-app-bar-mobile-row-height; | ||
| height: $mdc-top-app-bar-mobile-row-height; |
There was a problem hiding this comment.
I suspect this rule is also not needed? Both the non-mobile short row rule and the default mobile breakpoint rule already use this value for height.
fixes #2378
Adds prominent style.