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

chore: improve coverage and one minor bug fix #132

Merged
merged 1 commit into from
Apr 18, 2018
Merged

Conversation

picodoth
Copy link
Contributor

before:
before

after:
after

@picodoth picodoth requested a review from yesmeck April 16, 2018 12:22
@@ -175,22 +175,8 @@ const Menu = createReactClass({
return transitionName;
},

isInlineMode() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not used anywhere

return this.props.mode === 'inline';
},

lastOpenSubMenu() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not used anywhere

@@ -10,7 +10,7 @@ import { noop } from './util';

/* eslint react/no-is-mounted:0 */

const MenuItem = createReactClass({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exporting only for test

src/MenuMixin.js Outdated
@@ -106,7 +101,7 @@ const MenuMixin = {
},

// all keyboard events callbacks run from here at first
onKeyDown(e, callback) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

form onKeyDown(e, callback) is not used anywhere

Copy link
Member

Choose a reason for hiding this comment

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

callback 是给 rc-select 用的,这个 PR 的代码放 rc-select 里跑下 rc-select 的测试。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这么神奇 好的

src/MenuMixin.js Outdated
}

return 1;
} else if (activeItem === undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

activeItem is either null or an object, never hit undefined. null case will be taken care by caller.

@coveralls
Copy link

coveralls commented Apr 16, 2018

Coverage Status

Coverage increased (+10.5%) to 97.899% when pulling dabb945 on adding-test into 92e64ae on master.

@@ -154,25 +141,10 @@ const MenuMixin = {
},

getFlatInstanceArray() {
let instanceArray = this.instanceArray;
const hasInnerArray = instanceArray.some((a) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

since subIndex is always invalid, hasInnerArray will never be true.

@@ -3,7 +3,7 @@
exports[`Menu render renders horizontal menu correctly 1`] = `
<ul
aria-activedescendant=""
class="rc-menu myMenu rc-menu-root rc-menu-vertical"
class="rc-menu myMenu rc-menu-root rc-menu-horizontal"
Copy link
Member

Choose a reason for hiding this comment

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

这个为啥会有变化?看起来原来是错的?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yesmeck https://github.com/react-component/menu/pull/132/files#diff-ccd4e2639d6848cd0e377d2f79ad2178L36 这个地方的测试写的不对,测了vertical, horizontal, inline,但是实际上都默认为vertical了

Copy link
Member

Choose a reason for hiding this comment

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

👌

subMenuInstance.componentWillUnmount();

expect(onDestroy).toHaveBeenCalledWith('s1');
});
Copy link
Member

Choose a reason for hiding this comment

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

最好是写个组件去测试这种情况。

const App = () => (
  <Menu>
    {show && (
      <SubMenu key="s1" title="submenu1" onDestroy={onDestroy}>
        <MenuItem key="s1-1">1</MenuItem>
      </SubMenu>
    )}
  </Menu>
);

const wrapper = mount(<App show />);
wrapper.setProps({ show: false });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@yesmeck
Copy link
Member

yesmeck commented Apr 18, 2018

CI failed.

@picodoth picodoth force-pushed the adding-test branch 3 times, most recently from 334b9dc to e8f8130 Compare April 18, 2018 06:04
@picodoth
Copy link
Contributor Author

@yesmeck revert了关于callback的改动,把当前branch放到rc-select里跑了测试,没问题

@picodoth picodoth merged commit 6eb19f0 into master Apr 18, 2018
@picodoth picodoth deleted the adding-test branch April 18, 2018 06:56
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