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

fix(tab-indicator): update component to work with mdc tab #215

Merged
merged 5 commits into from
Aug 16, 2018

Conversation

moog16
Copy link

@moog16 moog16 commented Aug 9, 2018

  • removed component management of --active class
  • using foundation.active() method to manage --active class
  • using ref to directly set content style instead of state. Animation did not always show
  • put computeContentClientRect on component to be called by parent component (mdc-tab)
  • moved tab indicator content child to render within content element. This allows the content element to accept a ref, instead of trying to pass it to the child (code is a lot more complex).
    • this change requires us to have a override style for height/width on the --icon .material-icons element. Since .material-icons has a 24px font-size

@codecov-io
Copy link

codecov-io commented Aug 9, 2018

Codecov Report

Merging #215 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #215      +/-   ##
=========================================
+ Coverage   98.78%   98.8%   +0.01%     
=========================================
  Files          21      21              
  Lines         824     834      +10     
  Branches       76      78       +2     
=========================================
+ Hits          814     824      +10     
  Misses         10      10
Impacted Files Coverage Δ
packages/tab-indicator/index.js 100% <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 05afa91...e2fcaea. Read the comment docs.

@@ -1 +1,6 @@
@import "@material/tab-indicator/mdc-tab-indicator";

.mdc-tab-indicator__content--icon .material-icons {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont do this. Instead make the material-icons stylesheet load before the mdc-tab-indicator stylesheet

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the .material-icons class has the font-size: 24px. So rearranging the stylesheets will not help here.

I moved the logic around so that we now have the ref on the TabIndicatorElement. We then use .getElementsByClassName, which I think is fine in this case. The call is scoped to just the tabIndicator.

@moog16 moog16 merged commit bc41a8b into master Aug 16, 2018
@moog16 moog16 deleted the fix/tab-indicator/update-api branch August 16, 2018 06:39
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