-
Notifications
You must be signed in to change notification settings - Fork 232
feat(top app bar): dense variant #520
feat(top app bar): dense variant #520
Conversation
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.
Looks good just a few minor tweaks. I think we merge this PR in first, and then update your other one. I will add the screenshot tests. So you will need to update the golden file.
@@ -63,6 +63,11 @@ test('has correct fixed class', () => { | |||
assert.isTrue(wrapper.hasClass('mdc-top-app-bar--fixed')); | |||
}); | |||
|
|||
test('has correct dense class', () => { |
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.
can you also add one for dense prominent variant?
Codecov Report
@@ Coverage Diff @@
## rc0.8.0 #520 +/- ##
========================================
Coverage 96.62% 96.62%
========================================
Files 59 59
Lines 1984 1984
Branches 235 235
========================================
Hits 1917 1917
Misses 67 67
Continue to review full report at Codecov.
|
Please add this commit to your PR dc69fe1 |
Also you need to "sign" in a separate commit. The CLA hasn't passed. |
I signed it
@moog16 like this? this instruction is a little unclear to me. thanks for the review. |
tests passing #523 |
Woo! Thanks |
adds missing dense variant. See issue #518. As noted on discord I am having trouble getting screenshot testing to run. But I did add two. A
TopAppBarDense
and aTopAppBarProminetDense
. Both will be modified further in an additional PR that adds the correct class to<TopAppBarFixedAdjust>
I signed it.