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 menu mutli active item #1438

Merged
merged 7 commits into from
Jun 15, 2017
Merged

fix menu mutli active item #1438

merged 7 commits into from
Jun 15, 2017

Conversation

DavidSuns
Copy link
Contributor

@DavidSuns DavidSuns commented Jun 12, 2017

related to issue #1427


This change is Reviewable

@mention-bot
Copy link

@DavidSuns, thanks for your PR! By analyzing the history of the files in this pull request, we identified @warmhug, @yiminghe and @paranoidjk to be potential reviewers.

@codecov
Copy link

codecov bot commented Jun 12, 2017

Codecov Report

Merging #1438 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1438   +/-   ##
=======================================
  Coverage   63.44%   63.44%           
=======================================
  Files         222      222           
  Lines        4336     4336           
  Branches     1272     1271    -1     
=======================================
  Hits         2751     2751           
  Misses       1584     1584           
  Partials        1        1
Flag Coverage Δ
#rn 64.29% <ø> (ø) ⬆️
#web 62.62% <100%> (ø) ⬆️
Impacted Files Coverage Δ
components/menu/SubMenu.web.tsx 100% <100%> (ø) ⬆️
components/menu/index.web.tsx 91.8% <100%> (+2.67%) ⬆️

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 f0269a0...ac85810. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 12, 2017

Codecov Report

Merging #1438 into master will increase coverage by 1.59%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1438      +/-   ##
==========================================
+ Coverage   61.85%   63.45%   +1.59%     
==========================================
  Files         222      101     -121     
  Lines        4336     2126    -2210     
  Branches     1272      668     -604     
==========================================
- Hits         2682     1349    -1333     
+ Misses       1653      777     -876     
+ Partials        1        0       -1
Flag Coverage Δ
#rn 63.45% <ø> (ø) ⬆️
#web ?
Impacted Files Coverage Δ
components/popup/index.web.tsx
components/locale-provider/style/index.web.tsx
components/list-view/Indexed.web.tsx
components/textarea-item/index.web.tsx
components/modal/style/index.web.tsx
components/wing-blank/index.web.tsx
components/flex/Flex.web.tsx
components/radio/index.web.tsx
components/modal/alert.web.tsx
components/popover/Item.web.tsx
... and 109 more

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 97d9257...5e6e00e. Read the comment docs.

@paranoidjk
Copy link
Contributor

@DavidSuns You should write close #1427

@paranoidjk
Copy link
Contributor

@DavidSuns Please follow git-commit-guidelines

@@ -7,6 +7,7 @@ export interface DataItem {
isLeaf?: boolean;
disabled?: boolean;
[key: string]: any;
showSelect: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for ?

@@ -5,32 +5,14 @@ import List from '../list/index.web';
import Radio from '../radio/Radio.web';

export default class SubMenu extends React.Component<any, any> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This component is a stateless component, just write it with pure function.

@paranoidjk
Copy link
Contributor

@DavidSuns

Please fix the commit message and the review problem, then i am going to merge this.

@DavidSuns
Copy link
Contributor Author

@paranoidjk , i will update a commit today

  two menu bugs which are described in ant-design#1427

ant-design#1427
@@ -37,37 +39,47 @@ export default class Menu extends React.Component<MenuProps, any> {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

getNewFsv的写法也改写下吧,这个写法很难阅读。


let subMenuData = data[0].children || [];
if (level !== 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

subMenuData的处理逻辑也比较啰嗦,可以优化下。

@@ -97,6 +98,9 @@
&:only-child {
.@{listPrefixCls} {
.@{listPrefixCls}-item {
.@{listPrefixCls}-line {
.hairline-bottom(@border-color-base);
Copy link
Contributor

Choose a reason for hiding this comment

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

单级情况下,每个ListItem的下边框缺失

1. refactor function getNewFsv
2. refactor function render
3. fix css-lint error
const { firstLevelSelectValue, value } = this.state;
let subMenuData = data; // menu only has one level as init

if (level === 2) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

重新梳理了一下逻辑,加入了isLeaf === true 就不显示children 的判断

Copy link
Contributor

Choose a reason for hiding this comment

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

isLeaf的使用场景是干啥?demo里的isLeaf用来显示全部类型?也不太对啊

要问下这个组件最初的开发者

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个跟@pingan1927 沟通过。
isLeaf 表明它是叶子节点,无法展开二级节点,在文档中说,isLeaf的优先级大于children。之前代码中没有这个判断。
demo中叶子节点显示全部类型,是有些令人困惑,我去修正一下demo。

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

文档和 demo 有可以完善的地方就直接改掉

@paranoidjk paranoidjk changed the title fix menu bug fix menu mutli active item Jun 14, 2017
@paranoidjk
Copy link
Contributor

@DavidSuns
demo 里面 这个蒙版的样式会导致页面长度增长而可以上下滚动,你一起修复一下?

.menu-active:after {
  content: ' ';
  position: absolute;
  width: 100%;
  height: 100%;
  background-color: #000;
  opacity: 0.4;
  z-index: 1;
}

@DavidSuns
Copy link
Contributor Author

好,我明天看一下

@paranoidjk
Copy link
Contributor

@DavidSuns tnpm run test:web -- -u

@paranoidjk paranoidjk merged commit e77dfc7 into ant-design:master Jun 15, 2017
lixiaoyang1992 pushed a commit to lixiaoyang1992/ant-design-mobile that referenced this pull request Apr 26, 2018
* fix menu bug

* fix menu

  two menu bugs which are described in ant-design#1427

ant-design#1427

* improve menu style

* refactor menu

1. refactor function getNewFsv
2. refactor function render
3. fix css-lint error

* update snap and fix PropsType

* enhance menu demo

* 修复demo 蒙层撑开屏幕的问题
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.

4 participants