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(tab): Get tabs by their ID #4149

Merged
merged 6 commits into from
Dec 10, 2018
Merged

Conversation

patrickrodee
Copy link
Contributor

@patrickrodee patrickrodee commented Dec 5, 2018

Use unique identifiers to reference tabs instead of the object equality comparison. Change the getIndexOfTab method to getIndexOfTabByID. Update tests.

This makes it more feasible to wrap MDC Tab Bar for frameworks, which shouldn't need to reference the vanilla component.

BREAKING CHANGE: MDCTabBar#getIndexOfTab(tab: MDCTab): number is now MDCTabBar#getIndexOfTabByID(id: string): number

@codecov-io
Copy link

codecov-io commented Dec 5, 2018

Codecov Report

Merging #4149 into master will decrease coverage by 0.03%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4149      +/-   ##
=========================================
- Coverage   98.43%   98.4%   -0.04%     
=========================================
  Files         126     126              
  Lines        5616    5626      +10     
  Branches      753     755       +2     
=========================================
+ Hits         5528    5536       +8     
- Misses         88      90       +2
Impacted Files Coverage Δ
packages/mdc-tab/index.js 92.98% <100%> (+0.25%) ⬆️
packages/mdc-tab-bar/foundation.js 100% <100%> (ø) ⬆️
packages/mdc-tab-bar/index.js 85.71% <88.23%> (-1.25%) ⬇️

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 2f6dda2...93a240e. Read the comment docs.

packages/mdc-tab-bar/adapter.js Outdated Show resolved Hide resolved
packages/mdc-tab-bar/adapter.js Outdated Show resolved Hide resolved
packages/mdc-tab-bar/index.js Outdated Show resolved Hide resolved
test/unit/mdc-tab-bar/mdc-tab-bar.test.js Outdated Show resolved Hide resolved
@@ -63,6 +66,7 @@ class MDCTab extends MDCComponent {
initialize(
rippleFactory = (el, foundation) => new MDCRipple(el, foundation),
tabIndicatorFactory = (el) => new MDCTabIndicator(el)) {
this.id = this.root_.id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the element's id guaranteed to be set by the time this runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is guaranteed to be set. I based this off chips which is working appropriately.

packages/mdc-tab-bar/index.js Outdated Show resolved Hide resolved
packages/mdc-tab-bar/index.js Outdated Show resolved Hide resolved
@mdc-web-bot
Copy link
Collaborator

All 691 screenshot tests passed for commit 1e861da vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 691 screenshot tests passed for commit f5992c7 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 691 screenshot tests passed for commit 5df93de vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 691 screenshot tests passed for commit 93a240e vs. master! 💯🎉

@patrickrodee patrickrodee merged commit 2d35220 into master Dec 10, 2018
@patrickrodee patrickrodee deleted the feat/tabs/getIndexOfTabByID branch December 10, 2018 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants