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: prefixCls dosen't pass to Popconfirm's Button #12677

Merged
merged 2 commits into from Oct 19, 2018

Conversation

concefly
Copy link
Contributor

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 branch feature.
  • 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.

#12676

Add btnPrefixCls for Popconfirm's Button

@zombieJ
Copy link
Member

zombieJ commented Oct 16, 2018

ref: #10855
Should we both add btnPrefixCls on Popconfirm and Modal?

@concefly
Copy link
Contributor Author

concefly commented Oct 17, 2018

ref: #10855
Should we both add btnPrefixCls on Popconfirm and Modal?

@zombieJ

It seems that there are props which can assign prefixCls to Modal's Button.

okButtonProps?: NativeButtonProps;
cancelButtonProps?: NativeButtonProps;

@zombieJ
Copy link
Member

zombieJ commented Oct 17, 2018

Got it.
Let's follow modal use okButtonProps & cancelButtonProps instead.

@concefly
Copy link
Contributor Author

Got it.
Let's follow modal use okButtonProps & cancelButtonProps instead.

Sure, I'm going to commit again.

@codecov
Copy link

codecov bot commented Oct 17, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12677   +/-   ##
=======================================
  Coverage   92.34%   92.34%           
=======================================
  Files         211      211           
  Lines        5550     5550           
  Branches     1561     1600   +39     
=======================================
  Hits         5125     5125           
+ Misses        421      419    -2     
- Partials        4        6    +2
Impacted Files Coverage Δ
components/popconfirm/index.tsx 100% <100%> (ø) ⬆️
components/transfer/index.tsx 87.91% <0%> (ø) ⬆️
components/time-picker/index.tsx 81.96% <0%> (ø) ⬆️

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 bf83845...8de8d75. Read the comment docs.

title="x"
prefixCls="custom-popconfirm"
okButtonProps={{ prefixCls: btnPrefixCls }}
cancelButtonProps={{ prefixCls: btnPrefixCls }}
Copy link
Member

@yesmeck yesmeck Oct 17, 2018

Choose a reason for hiding this comment

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

A little....too verbose, can we introduce a context provider for prefixCls?

Copy link
Member

Choose a reason for hiding this comment

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

I think keep it is OK. Though if user want prefix root with 'abc' but inner component with 'def'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most off all, we only need a single prefixCls in a project. So I think that context provider is much better.

exp:

<PrefixClsProvider prefixCls='custom'>
  <Button />
</PrefixClsProvider>

Then the Button will have default prefixCls with 'custom'

Copy link
Member

Choose a reason for hiding this comment

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

OK. I guess we can close this PR and open a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... this PR is a bug fix so maybe we could merge it and then make a new feature PR for PrefixClsProvider ?

Copy link
Member

Choose a reason for hiding this comment

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

这个先合并吧,不过这个是新增了属性,算 feature 了,要 PR 到 feature 分支。

@concefly
Copy link
Contributor Author

Should we marge this PR?

@@ -106,10 +108,10 @@ class Popconfirm extends React.Component<PopconfirmProps, PopconfirmState> {
<div className={`${prefixCls}-message-title`}>{title}</div>
</div>
<div className={`${prefixCls}-buttons`}>
<Button onClick={this.onCancel} size="small">
<Button onClick={this.onCancel} size="small" {...(cancelButtonProps || {})}>
Copy link
Member

Choose a reason for hiding this comment

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

直接 {...cancelButtonProps} 就可以了。

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 yesmeck merged commit 057c8a3 into ant-design:master Oct 19, 2018
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

3 participants