-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(tabs): Implement a tab component #581
Conversation
Codecov Report
@@ Coverage Diff @@
## master #581 +/- ##
==========================================
+ Coverage 99.91% 99.91% +<.01%
==========================================
Files 53 59 +6
Lines 2224 2457 +233
Branches 267 284 +17
==========================================
+ Hits 2222 2455 +233
Misses 2 2
Continue to review full report at Codecov.
|
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 really good! Just added a few changes needed; most comments have to do with the tests. Btw, this PR should be rebased and merged. Ideally there would be three commits:
- Changes to mdc-base
- Changes to mdc-toolbar
- TabBar/Tabs Feature
Thanks! ✨
demos/tabs.html
Outdated
<link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons"> | ||
<script src="assets/material-components-web.css.js" charset="utf-8"></script> | ||
<style> | ||
.demo-body { |
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.
nit: indentation is off here
demos/tabs.html
Outdated
|
||
<section> | ||
<h2>CSS-Only Icon & Text Labels</h2> | ||
<nav id="icon-text-tab-bar" class="mdc-tab-bar mdc-tab-bar--icons-with-text"> |
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.
nit: duplicate id
here
packages/mdc-tabs/tab/mdc-tab.scss
Outdated
} | ||
} | ||
|
||
.mdc-tab-bar:not(.mdc-tab-bar-upgraded) .mdc-tab--with-icon-and-text { |
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.
Could we just use .mdc-tab-bar--icons-with-text:not(.mdc-tab-bar-upgraded) .mdc-tab::after
here? This way we don't need an additional modifier class on mdc-tab
const transformValue = `translateX(${translateAmtForActiveTabLeft}px) scale(${scaleAmtForActiveTabWidth}, 1)`; | ||
|
||
['-webkit-transform', 'transform'].forEach((transformProperty) => { | ||
this.adapter_.setStyleForIndicator(transformProperty, transformValue); |
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.
could we use mdc-animation's getPropertyName
here?
demos/tabs.html
Outdated
var dotIndex = [].slice.call(dots.querySelectorAll('.dot')).indexOf(evt.target); | ||
|
||
if (dotIndex >= 0) { | ||
dynamicTabBar.foundation_.switchToTabAtIndex(dotIndex, false); |
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.
🚫 This is referencing a private (protected) property!
IIRC the prototype code had getter/setter component properties activeTab
and activeTabIndex
that could be used for this purpose. Those should be implemented at the component level and proxy to the foundation.
test/unit/mdc-tabs/mdc-tab.test.js
Outdated
}); | ||
|
||
if (supportsCssVariables(window)) { | ||
test('#destroy cleans up ripple on tab', () => { |
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.
we may also want to add a test verifying that the ripple is instantiated
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.
👍
test/unit/mdc-tabs/mdc-tab.test.js
Outdated
const {component} = setupTest(); | ||
|
||
component.isActive = false; | ||
assert.isFalse(component.root_.classList.contains(cssClasses.ACTIVE)); |
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.
use root
from setupTest()
instead of component.root_
test/unit/mdc-tabs/mdc-tab.test.js
Outdated
assert.isTrue(component.root_.classList.contains(cssClasses.ACTIVE)); | ||
}); | ||
|
||
test('#get/set preventDefaultOnClick', () => { |
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.
this should also test that the foundation logic from preventDefaultOnClick
kicks in
test/unit/mdc-tabs/mdc-tab.test.js
Outdated
const {root, component} = setupTest(); | ||
|
||
root.classList.add('foo'); | ||
assert.isTrue(root.classList.contains('foo')); |
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.
I don't think is necessary to test that DOMTokenList.prototype.add is working correctly 😉
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.
D'oh!
👍
test/unit/mdc-tabs/mdc-tab.test.js
Outdated
root.addEventListener(type, handler); | ||
|
||
component.getDefaultFoundation().adapter_.deregisterInteractionHandler(type, handler); | ||
domEvents.emit(root, 'click'); |
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.
type
eef7592
to
4e05689
Compare
demos/tabs.html
Outdated
</section> | ||
|
||
<section> | ||
<h2>CSS-Only Icon Labels</h2> |
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.
"Icon Tab Labels"?
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.
That sounds fine. What about keeping CSS-Only? I think it implies that the best experience will be with the fully upgraded JavaScript component, and that sits well with me.
"CSS-Only Icon Tab Labels" ?
packages/mdc-tabs/README.md
Outdated
|
||
### Tab component API | ||
|
||
#### MDCTab.destroy() => void |
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.
We don't need to document this since it's a method within MDCComponent
. We do need to document the active
getter/setter as well as the preventDefaultonClick
getter/setter.
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.
I added a setter/getter
section to the component API docs to address this. Let me know if there is a more preferred notation. I couldn't really find any examples of documenting instances of public getters AND setters. This seems logical to me
packages/mdc-tabs/README.md
Outdated
|
||
### Tab Bar component API | ||
|
||
#### MDCTabBar.initialize() => void |
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.
Same comment as above. We need to document all of public getter/setters and non-inherited methods (such as layout()
) here as well.
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.
👍
packages/mdc-tabs/tab-bar/index.js
Outdated
} | ||
|
||
get activeTab() { | ||
return this.tabs[this.foundation_.activeTabIndex_]; |
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.
How about this.tabs[this.activeTabIndex]
?
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.
I think that will result in a circular reference with get activeTabIndex
. What might be required is a public method like getActiveTabIndex
in the foundation which calls activeTabIndex_
.
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.
Implemented, tested, and documented.
packages/mdc-tabs/tab-bar/index.js
Outdated
} | ||
|
||
set activeTab(tab) { | ||
this.foundation_.switchToTabAtIndex(this.tabs.indexOf(tab), true); |
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.
How about this.setActiveTab_(tab, false)
?
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.
👍
</nav> | ||
``` | ||
|
||
### Using the JavaScript Component |
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.
We should, at some point in this section, talk about tabFactory
as a way of controlling how MDCTabBar
instantiates its MDCTab
components.
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.
👍
packages/mdc-tabs/tab/mdc-tab.scss
Outdated
padding: 0 12px; | ||
} | ||
|
||
// postcss-bem-linter: ignore |
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.
why does this need to be ignored?
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.
It doesn't
foundation.init(); | ||
raf.flush(); | ||
|
||
td.verify(mockAdapter.setStyleForIndicator(td.matchers.anything(), td.matchers.anything())); |
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.
this doesn't really verify that -webkit-transform
and transform
are set. It just verifies that setStyleForIndicator
is called for some set of two arguments
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.
Correct. I added two additional verifications as well for all of the styles init()
sets on the indicator.
const tab = component.tabs[targetIndex]; | ||
|
||
component.activeTab = tab; | ||
td.verify(foundation.switchToTabAtIndex(targetIndex, td.matchers.isA(Boolean))); |
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.
I'd rather the foundation not be mocked here and we check that the correct classes are added to the mock tab element.
const component = new MDCTabBar(root, foundation); | ||
|
||
component.activeTabIndex = 1; | ||
td.verify(foundation.switchToTabAtIndex(1, td.matchers.isA(Boolean))); |
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.
same comment as above
demos/index.html
Outdated
<span class="catalog-list-icon mdc-list-item__start-detail"><img class="catalog-component-icon" src="/images/ic_tabs_24px.svg" /></span> | ||
<a href="tabs.html" class="mdc-list-item__text"> | ||
Tabs | ||
<span class="mdc-list-item__text__secondary">Tabs with icon and text labels</span> |
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.
nit: "with support for"?
@@ -0,0 +1,549 @@ | |||
<!DOCTYPE html> |
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.
As discussed offline this should be updated to match our new demo ui. So sorry to toss this at you 😢
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.
Not a problem
packages/mdc-tabs/README.md
Outdated
} | ||
} | ||
|
||
dynamicTabs.root_.addEventListener('MDCTabBar:change', function ({detail: tabs}) { |
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.
This should be dynamicTabs.listen('MDCTabBar:change', ...)
demos/tabs.html
Outdated
} | ||
} | ||
|
||
dynamicTabBar.root_.addEventListener('MDCTabBar:change', function (t) { |
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.
dynamicTabBar.listen
. Please don't access private properties outside of the entity their defined in.
packages/mdc-tabs/README.md
Outdated
var dotIndex = [].slice.call(dots.querySelectorAll('.dot')).indexOf(evt.target); | ||
|
||
if (dotIndex >= 0) { | ||
dynamicTabBar.foundation_.switchToTabAtIndex(dotIndex, false); |
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.
⛔️ 🚫 🙅♂️
packages/mdc-tabs/tab-bar/index.js
Outdated
} | ||
|
||
set activeTabIndex(index) { | ||
this.foundation_.switchToTabAtIndex(index, true); |
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.
this second argument should be false
because we don't want to trigger an event when tabs are programmatically set.
Also, you have a helper method in this file called setActiveTabIndex_
. Could we just use that instead?
packages/mdc-tabs/tab/index.js
Outdated
this.foundation_.setPreventDefaultOnClick(preventDefaultOnClick); | ||
} | ||
|
||
get ripple() { |
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.
OOC why is this needed?
@@ -44,13 +44,14 @@ $mdc-toolbar-mobile-breakpoint: 599px; | |||
display: flex; |
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.
will you be rebase-and-merge'ing these changes?
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.
Yes. AFAIK they are only necessary for toolbars with tabs inside of them, so this wouldn't have been fixed anywhere else.
assert.equal(tab, component.activeTab); | ||
}); | ||
|
||
test('#set activeTab proxies to foundation_.switchToTabAtIndex', () => { |
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.
(here and below) rather than saying "proxies to foundation_.", which describes an implementation detail not seen in this test, it would be better to describe what the implementation is trying to accomplish
assert.throws(() => component.tabSelectedHandler_(evtObj)); | ||
}); | ||
|
||
test('adapter#unbindOnMDCTabSelectedEvent removes listener from component', () => { |
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.
I don't think this is testing what you think.
You're listening for MDCTab:selected
on the component but firing it on tab.root
. Are you sure the event is propagating up to the component level? Because if so that handler should be called, right?
You may instead want to use a similar approach above, and check that detail.tab
is not active after the event is emitted on the component's root element.
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.
You are right. Verified that the handler was getting called. Switched to the approached used in bindOn...
…omponents-web into feat/tabs
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.
🥇 🏁 Once CI passes let's finally merge this bad boy
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
resolves #581