Skip to content
This repository was archived by the owner on Jul 29, 2025. It is now read-only.

Conversation

bonniezhou
Copy link
Contributor

Fixes #279 and #276

@bonniezhou bonniezhou requested a review from moog16 September 24, 2018 20:41
@codecov-io
Copy link

codecov-io commented Sep 24, 2018

Codecov Report

Merging #280 into master will increase coverage by 0.08%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
+ Coverage   97.21%   97.29%   +0.08%     
==========================================
  Files          33       33              
  Lines        1257     1258       +1     
  Branches      122      122              
==========================================
+ Hits         1222     1224       +2     
+ Misses         35       34       -1
Impacted Files Coverage Δ
packages/tab/TabRipple.js 87.5% <ø> (+17.5%) ⬆️
packages/tab-bar/index.js 76.19% <0%> (-1.23%) ⬇️
packages/tab/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 194085d...b7608c2. Read the comment docs.

this.tabList_.forEach((tabList, index) => {
if (tabList.tabElement_.current === activeElement) {
return index;
for (let i = 0; i < this.tabList_.length; i++) {
Copy link

Choose a reason for hiding this comment

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

I don't think you need this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what's causing #276, the forEach is returning undefined

Copy link

Choose a reason for hiding this comment

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

ahh gotcha - lets just leave a comment so no one changes it back to a forEach or something

@@ -305,3 +305,25 @@ test('#componentWillUnmount destroys foundation', () => {
wrapper.unmount();
td.verify(foundation.destroy(), {times: 1});
});

test('on focus event calls handleFocus on TabRipple', () => {
const wrapper = shallow(<Tab />);
Copy link

Choose a reason for hiding this comment

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

It looks like you're trying to get the ref here...you can just use mount instead of shallow. Then these 3 lines can just be:

const wrapper = mount(<Tab />);
const ripple = wrapper.instance(). tabRipple_.current;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

});

test('on blur event calls handleBlur on TabRipple', () => {
const wrapper = shallow(<Tab />);
Copy link

Choose a reason for hiding this comment

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

same here

this.tabList_.forEach((tabList, index) => {
if (tabList.tabElement_.current === activeElement) {
return index;
for (let i = 0; i < this.tabList_.length; i++) {
Copy link

Choose a reason for hiding this comment

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

ahh gotcha - lets just leave a comment so no one changes it back to a forEach or something

@bonniezhou
Copy link
Contributor Author

Good call 👍

@bonniezhou bonniezhou merged commit 54224c8 into master Sep 24, 2018
@bonniezhou bonniezhou deleted the fix/tab/keydown branch September 24, 2018 23:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants