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

feat: Modal.confirm add maskClosable option (#4488) #4490

Merged
merged 2 commits into from
Jan 7, 2017

Conversation

pixystone
Copy link
Contributor

close #4488

@mention-bot
Copy link

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

@@ -21,6 +21,7 @@ function showConfirm() {
confirm({
title: 'Want to delete these items?',
content: 'some descriptions',
maskClosable: true,
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.

不太清楚test case哪里加

Copy link
Member

Choose a reason for hiding this comment

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

@@ -16,6 +16,9 @@ export default function confirm(config) {
let width = props.width || 416;
let style = props.style || {};

// 默认为 false,保持旧版默认行为
let maskClosable = ('maskClosable' in props) && props.maskClosable || false;
Copy link
Member

Choose a reason for hiding this comment

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

let => const

Copy link
Member

@afc163 afc163 Jan 5, 2017

Choose a reason for hiding this comment

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

? : 三目运算符可读性会更好一点。

@afc163
Copy link
Member

afc163 commented Jan 5, 2017

好快,给力。:+1:

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 71.44% when pulling ef00946 on pixystone:feature-2.7 into e54da6e on ant-design:feature-2.7.

@benjycui
Copy link
Contributor

benjycui commented Jan 6, 2017

👍 按 review 的改完,就可以合并了。

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 71.44% when pulling a547503 on pixystone:feature-2.7 into e54da6e on ant-design:feature-2.7.

@@ -16,6 +16,9 @@ export default function confirm(config) {
let width = props.width || 416;
let style = props.style || {};

// 默认为 false,保持旧版默认行为
const maskClosable = props.maskClosable === undefined ? false : props.maskClosable;
Copy link
Member

Choose a reason for hiding this comment

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

应该用 defaultProps 吧。

Copy link
Member

Choose a reason for hiding this comment

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

方法调用没有 defaultProps 的。

Copy link
Member

Choose a reason for hiding this comment

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

奥,这个是方法,那写到上面的 assgin 里跟 iconType 一起好点?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

还需要改么?这里我先跟width和style上面的保持一致,下面还有一些okCancel之类的配置其实也不是很明显

@afc163 afc163 merged commit b949d88 into ant-design:feature-2.7 Jan 7, 2017
@afc163
Copy link
Member

afc163 commented Jan 7, 2017

👍

afc163 pushed a commit that referenced this pull request Jan 8, 2017
* feat: Modal.confirm add maskClosable option (#4488)

* feat: Modal.confirm add maskClosable option (#4488)
@pixystone pixystone deleted the feature-2.7 branch January 9, 2017 14:30
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.

None yet

6 participants