feat(top-app-bar): Add --fixed variant to top app bar #2474
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2474 +/- ##
==========================================
+ Coverage 98.67% 98.71% +0.03%
==========================================
Files 102 103 +1
Lines 4090 4111 +21
Branches 512 516 +4
==========================================
+ Hits 4036 4058 +22
+ Misses 54 53 -1
Continue to review full report at Codecov.
|
| @@ -26,6 +26,8 @@ const strings = { | |||
|
|
|||
| /** @enum {string} */ | |||
| const cssClasses = { | |||
| FIXED_CLASS: 'mdc-top-app-bar--fixed', | |||
| FIXED_SCROLLED_CLASS: 'mdc-top-app-bar--fixed__scrolled', | |||
There was a problem hiding this comment.
In BEM, __ indicates a sub-element, but this appears to be a modifier class (for a modifier class - yo dawg).
We normally handle that by using a single - instead of __.
E.g.: mdc-top-app-bar--fixed-scrolled
| */ | ||
| constructor(adapter) { | ||
| super(adapter); | ||
| // State variable for the previous top app bar state |
There was a problem hiding this comment.
Pro ~~~tip~~~ nit: Always use /** comments to document fields and methods.
Editors like IntelliJ will parse the JSDoc /** comments and display them inline when you hit Ctrl+Q.
| @@ -145,5 +145,13 @@ | |||
| } | |||
| } | |||
|
|
|||
| .mdc-top-app-bar--fixed { | |||
| position: fixed; | |||
There was a problem hiding this comment.
We probably want some kind of transition for box-shadow to make the transition between --fixed and --fixed-scrolled a little smoother.
demos/top-app-bar.html
Outdated
| }); | ||
|
|
||
| fixedCheckbox.addEventListener('change', function() { | ||
| appBarEl.classList[this.checked ? 'add' : 'remove']('mdc-top-app-bar--fixed'); |
There was a problem hiding this comment.
Nit: You might also want to toggle/remove mdc-top-app-bar--fixed-scrolled here, if it's not too hard to do.
Fixes: #2425