Skip to content
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

feat(top app bar): dense variant #520

Merged

Conversation

mgr34
Copy link
Contributor

@mgr34 mgr34 commented Dec 13, 2018

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 a TopAppBarProminetDense. Both will be modified further in an additional PR that adds the correct class to <TopAppBarFixedAdjust>


I signed it.

@mgr34 mgr34 changed the title Feat/top app bar/dense variant Feat(top app bar): dense variant Dec 14, 2018
@mgr34 mgr34 changed the title Feat(top app bar): dense variant feat(top app bar): dense variant Dec 14, 2018
Copy link

@moog16 moog16 left a 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.

test/screenshot/top-app-bar/prominentDense.js Show resolved Hide resolved
test/screenshot/top-app-bar/dense.js Outdated Show resolved Hide resolved
test/screenshot/top-app-bar/prominentDense.js Outdated Show resolved Hide resolved
@@ -63,6 +63,11 @@ test('has correct fixed class', () => {
assert.isTrue(wrapper.hasClass('mdc-top-app-bar--fixed'));
});

test('has correct dense class', () => {
Copy link

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-io
Copy link

codecov-io commented Dec 14, 2018

Codecov Report

Merging #520 into rc0.8.0 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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
Impacted Files Coverage Δ
packages/top-app-bar/index.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33fb500...a26a65d. Read the comment docs.

@moog16
Copy link

moog16 commented Dec 14, 2018

Please add this commit to your PR dc69fe1

@moog16
Copy link

moog16 commented Dec 14, 2018

Also you need to "sign" in a separate commit. The CLA hasn't passed.

@mgr34
Copy link
Contributor Author

mgr34 commented Dec 14, 2018

I signed it

Also you need to "sign" in a separate commit. The CLA hasn't passed.

@moog16 like this? this instruction is a little unclear to me.

thanks for the review.

@moog16
Copy link

moog16 commented Dec 14, 2018

tests passing #523

@moog16 moog16 merged commit 4a4e941 into material-components:rc0.8.0 Dec 14, 2018
@moog16
Copy link

moog16 commented Dec 14, 2018

Woo! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants