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.

Sign up for GitHub

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

Jump to bottom

filter the disabled options #4981

Merged
merged 10 commits into from
Feb 24, 2017
Merged

filter the disabled options #4981

merged 10 commits into from
Feb 24, 2017

Conversation

tianlizhao
Copy link
Contributor

@tianlizhao tianlizhao commented Feb 21, 2017 •
edited
Loading

First of all, thanks for your contribution! :-)

Please makes sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow antd's code convention.
  • Run npm run lint and fix those errors before submitting in order to keep consistent code style.
  • Rebase before creating a PR to keep commit history clear.
  • Add some descriptions and refer relative issues for you PR.
    1、修复Transfer穿梭框这个组件，当选择已经被checked的时候，通过事件disable，然后还可以移动到左/右边

@mention-bot
Copy link

@tianlizhao, thanks for your PR! By analyzing the history of the files in this pull request, we identified @afc163, @benjycui and @dqaria to be potential reviewers.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.962% when pulling e2e2aec on tianlizhao:master into 54dde4f on ant-design:master.

afc163
afc163 reviewed Feb 21, 2017
View reviewed changes
components/transfer/index.tsx Outdated
// filter the disabled options
const newMoveKeys = moveKeys.filter(key => {
return !dataSource.some(data => !!(key === data.key && data.disabled));
});
Copy link
Member

Choose a reason for hiding this comment

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

const newMoveKeys = moveKeys.filter(key => !dataSource.some(data => key === data.key && data.disabled));

@afc163
Copy link
Member

afc163 commented Feb 21, 2017

image

@tianlizhao
Copy link
Contributor Author

我要设置下我的email么？

@afc163
Copy link
Member

afc163 commented Feb 21, 2017

否则无法出现在贡献者里。https://github.com/ant-design/ant-design/graphs/contributors

@tianlizhao
Copy link
Contributor Author

好的，我把我邮箱加上了

@afc163
Copy link
Member

afc163 commented Feb 21, 2017

并没有变化：34cfd9f

afc163
afc163 requested changes Feb 21, 2017
View reviewed changes
Copy link
Member

@afc163 afc163 left a comment

Choose a reason for hiding this comment

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

  1. 邮箱无法识别。
  2. 按评论简化下代码。
  3. 如果有时间的话，麻烦写一个用例如何？

@afc163
Copy link
Member

afc163 commented Feb 21, 2017 

components/transfer/index.tsx(164,65): error TS2345: Argument of type '(data: TransferItem) => boolean | undefined' is not assignable to parameter of type '(value: TransferItem, index: number, array: TransferItem[]) => boolean'.

  Type 'boolean | undefined' is not assignable to type 'boolean'.

    Type 'undefined' is not assignable to type 'boolean'.

typescript 语法有问题。

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.962% when pulling 882b728 on tianlizhao:master into 54dde4f on ant-design:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.962% when pulling 6c37bc7 on tianlizhao:master into 54dde4f on ant-design:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.962% when pulling 2fdea13 on tianlizhao:master into 54dde4f on ant-design:master.

@tianlizhao
Copy link
Contributor Author

实在搞不明白了，我改了几次，每次都会有一个test文件不通过，我该怎么办呢？
我在本地npm run lint没有报错

@tianlizhao
Copy link
Contributor Author

可以写一个用例，有没有模板呢？写在哪里？

@afc163
Copy link
Member

afc163 commented Feb 21, 2017

__test__ 目录下就是。

@afc163
Copy link
Member

afc163 commented Feb 21, 2017

test 不过的原因在这里：79f222b

主干已修复，rebase 一下即可。

@benjycui
Copy link
Contributor

Why did this happen?

Actually, disabled options should not be selectable...

@benjycui
Copy link
Contributor

And it's not selectable https://ant.design/components/transfer/#components-transfer-demo-basic

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.962% when pulling 7131766 on tianlizhao:master into 90699b0 on ant-design:master.

@tianlizhao
Copy link
Contributor Author

@benjycui 正常情况下disabled的选项是不可以选中的。只有当选项被选中后，再通过别的组件调用了render方法，动态disabled的时候会出现这样的问题。
如下：
左边选项是根据region动态改变disabled
image
如果先选择了option,再通过region过滤的话，就会出现已经被勾选的被disabled
image
但是可以正常移动到右边
image

我也想过是否可以在render方法里把disabled的option去掉选中状态，但是看API和源码，没有发现可以在render里设置，好像只可以用selectedKeys也自己维护了。
按照正常逻辑的话，我还是觉得被disabled的option是不可以移动的，不管是不是被勾选状态。

benjycui
benjycui previously requested changes Feb 22, 2017
View reviewed changes
Copy link
Contributor

@benjycui benjycui left a comment

Choose a reason for hiding this comment

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

补充用例后就可以合了。

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.962% when pulling 7d2c8e4c74a6da5234f0708e6b57f01a700b15d7 on tianlizhao:master into 90699b0 on ant-design:master.

@benjycui
Copy link
Contributor

你这样修改单测，和你这次逻辑的修改没关系吧。

把这个单测修改回滚。

新增一个单测，其实就是把你 #4981 (comment) 说你话转成测试代码就可以了。

Thanks

然后不急的，周末才发版本，慢慢来吧

@tianlizhao
Copy link
Contributor Author

@benjycui 嗯嗯。好的，改的时候也觉得不太对，哈哈，好的，闲了我重写下

@benjycui benjycui changed the title filter the disabled options [WIP] filter the disabled options Feb 23, 2017
@benjycui
Copy link
Contributor

@tianlizhao just remove [WIP] in title when you have done.

@afc163
Copy link
Member

afc163 commented Feb 24, 2017

@tianlizhao any progress?

@tianlizhao
Copy link
Contributor Author

今天下班回家弄吧，是周末才发吧？还是今天 ？

@afc163
Copy link
Member

afc163 commented Feb 24, 2017

2.8 没这么快发，今天会发 master。不过你这个就差个用例了，估计花不了半小时。

@tianlizhao
Copy link
Contributor Author

好的，我现在试试

afc163
afc163 reviewed Feb 24, 2017
View reviewed changes
components/transfer/__tests__/index.test.js Outdated
disabled: true,
}],
selectedKeys: ['a'],
targetKeys: [],
Copy link
Member

Choose a reason for hiding this comment

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

可以多加一个不是 disabled 的 b 数据，保证 b 会被移动过去。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

@tianlizhao
Copy link
Contributor Author

提交了，不过写的有点不好，本来想共享 listCommonProps 变量的，没找到如何在test case里面把一个checkbox 动态disabeld掉，只能自己再写一个listDisabledProps，一开始就disabled=true，如果有更好的方式，请告诉我，谢谢了。

@afc163
Copy link
Member

afc163 commented Feb 24, 2017

没关系，分开也清晰点。

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.962% when pulling ea094f4 on tianlizhao:master into 2bf6e22 on ant-design:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.962% when pulling ea094f4 on tianlizhao:master into 2bf6e22 on ant-design:master.

@afc163 afc163 changed the title [WIP] filter the disabled options filter the disabled options Feb 24, 2017
@afc163 afc163 merged commit 85ca5ed into ant-design:master Feb 24, 2017
@afc163
Copy link
Member

afc163 commented Feb 24, 2017

Great job!!! @tianlizhao

@benjycui
Copy link
Contributor

Excellent 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afc163 afc163 afc163 approved these changes

@benjycui benjycui benjycui left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tianlizhao @mention-bot @coveralls @afc163 @benjycui