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

Allow to set type of Popconfirm OK button #6848

Merged
merged 10 commits into from
Jul 18, 2017
Merged

Allow to set type of Popconfirm OK button #6848

merged 10 commits into from
Jul 18, 2017

Conversation

yociduo
Copy link
Contributor

@yociduo yociduo commented Jul 16, 2017 •
edited
Loading

First of all, thank you for your contribution! :-)

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

  • Make sure that you propose PR to right branch: bugfix for master, feature for latest active branch feature-x.x.
  • Make sure that 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.

Extra checklist:

if isBugFix :

  • Make sure that you add at least one unit test for the bug which you had fixed.

elif isNewFeature :

  • Update API docs for the component.
  • Update/Add demo to demonstrate new feature.
  • Update TypeScript definition for the component.
  • Add unit tests for the feature.

@mention-bot
Copy link

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

@yociduo
Copy link
Contributor Author

yociduo commented Jul 16, 2017

Link Issue #6840

afc163
afc163 reviewed Jul 16, 2017
View reviewed changes
components/popconfirm/index.en-US.md Outdated
| cancelText| text of the cancel button | string | Cancel |
| cancelType| type of the confirmation button | string | none |
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need a cancelType, let's keep it as simple as possible.

Copy link
Member

Choose a reason for hiding this comment

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

Remove cancelType.

@afc163
Copy link
Member

afc163 commented Jul 16, 2017

Modal.confirm would need this feature too.

@yociduo
Copy link
Contributor Author

yociduo commented Jul 16, 2017

Modal.info need the feature? @afc163

@afc163
Copy link
Member

afc163 commented Jul 16, 2017

Nope.

@codecov
Copy link

codecov bot commented Jul 16, 2017

Codecov Report

Merging #6848 into feature-2.13 will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##           feature-2.13    #6848   +/-   ##
=============================================
  Coverage         86.02%   86.02%           
=============================================
  Files               231      231           
  Lines              4917     4917           
  Branches           1406     1406           
=============================================
  Hits               4230     4230           
  Misses              687      687
Impacted Files Coverage Δ
components/popconfirm/index.tsx 76.59% <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 81a010e...c95f84e. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 16, 2017 •
edited
Loading

Codecov Report

Merging #6848 into feature-2.13 will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##           feature-2.13    #6848   +/-   ##
=============================================
  Coverage         86.06%   86.06%           
=============================================
  Files               231      231           
  Lines              4957     4957           
  Branches           1416     1416           
=============================================
  Hits               4266     4266           
  Misses              691      691
Impacted Files Coverage Δ
components/modal/ActionButton.tsx 77.14% <100%> (ø) ⬆️
components/popconfirm/index.tsx 76.59% <100%> (ø) ⬆️
components/modal/confirm.tsx 87.87% <100%> (ø) ⬆️
components/modal/Modal.tsx 85% <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 8a63b39...2854151. Read the comment docs.

@yociduo
Copy link
Contributor Author

yociduo commented Jul 16, 2017

@afc163 Added

@afc163
Copy link
Member

afc163 commented Jul 16, 2017

Could you rebase the feature-2.13?

@yociduo
Copy link
Contributor Author

yociduo commented Jul 16, 2017

Ok

@afc163
Copy link
Member

afc163 commented Jul 16, 2017

Please make sure travis ci is passed~

@yociduo
Copy link
Contributor Author

yociduo commented Jul 17, 2017

Passed

@benjycui
Copy link
Contributor

@afc163 : I don't think we need a cancelType, let's keep it as simple as possible.

@yociduo

@yociduo
Copy link
Contributor Author

yociduo commented Jul 17, 2017

@benjycui remove cancel type, but I think cancel type is useful, because people can't override the footer in Popconfirm

@afc163
Copy link
Member

afc163 commented Jul 17, 2017

I don't think we need another type of cancel type but default type.

@yociduo
Copy link
Contributor Author

yociduo commented Jul 17, 2017

Ok, removed

benjycui
benjycui reviewed Jul 17, 2017
View reviewed changes
components/popconfirm/demo/basic.md Outdated
@@ -27,7 +27,7 @@ function cancel(e) {
}

ReactDOM.render(
<Popconfirm title="Are you sure delete this task?" onConfirm={confirm} onCancel={cancel} okText="Yes" cancelText="No">
<Popconfirm title="Are you sure delete this task?" onConfirm={confirm} onCancel={cancel} okText="Yes" okType="danger" cancelText="No" cancelType="dashed">
Copy link
Contributor

Choose a reason for hiding this comment

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

You forget to update demo

benjycui
benjycui reviewed Jul 17, 2017
View reviewed changes
components/modal/index.en-US.md Outdated
| cancelText | Text of cancel button | string | Cancel |
| cancelType| type of the confirmation button | string | none |
Copy link
Contributor

Choose a reason for hiding this comment

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

And docs ....

Just grep -r 'cancelType' ./components to find all the cancelType and remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, updated

afc163
afc163 reviewed Jul 17, 2017
View reviewed changes
components/popconfirm/demo/basic.md Outdated
@@ -27,7 +27,7 @@ function cancel(e) {
}

ReactDOM.render(
<Popconfirm title="Are you sure delete this task?" onConfirm={confirm} onCancel={cancel} okText="Yes" cancelText="No">
<Popconfirm title="Are you sure delete this task?" onConfirm={confirm} onCancel={cancel} okText="Yes" okType="danger" cancelText="No">
Copy link
Member

Choose a reason for hiding this comment

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

这个例子不要改。

@afc163 afc163 merged commit bf76d12 into ant-design:feature-2.13 Jul 18, 2017
@benjycui benjycui mentioned this pull request Jul 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afc163 afc163 afc163 left review comments

@benjycui benjycui benjycui approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@yociduo @mention-bot @afc163 @benjycui