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

Badge: Add ReacNode support for count prop #12140

Closed
wants to merge 1 commit into from

Conversation

supra28
Copy link
Contributor

@supra28 supra28 commented Sep 7, 2018

Fixes issue

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.

@ant-design-bot
Copy link
Contributor

ant-design-bot commented Sep 7, 2018

Deploy preview for ant-design ready!

Built with commit df191ad

https://deploy-preview-12140--ant-design.netlify.com

@supra28
Copy link
Contributor Author

supra28 commented Sep 7, 2018

@afc163 Is the CI broken?

@supra28 supra28 changed the title Badge: Add ReacNode to support for count prop Badge: Add ReacNode support for count prop Sep 7, 2018
@supra28
Copy link
Contributor Author

supra28 commented Sep 8, 2018

rebased after snapshot updates

@zombieJ
Copy link
Member

zombieJ commented Sep 12, 2018

It's better to add a demo about it : )

const state = this.state;
if (!state.count || isNaN(state.count as number)) {
if(props.displayComponent){
Copy link
Member

Choose a reason for hiding this comment

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

code style is a mess...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What exactly should I change here? It was already written this way.
This is my first time, pls give a few pointers.

Copy link
Member

Choose a reason for hiding this comment

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

- if(props.displayComponent){
+ if (props.displayComponent) {

@@ -105,7 +111,8 @@ export default class Badge extends React.Component<BadgeProps, any> {
data-show={!hidden}
className={scrollNumberCls}
count={displayCount}
title={title || count}
displayComponent = {displayComponent}
Copy link
Member

Choose a reason for hiding this comment

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

- displayComponent = {displayComponent}
+ displayComponent={displayComponent}

const state = this.state;
if (!state.count || isNaN(state.count as number)) {
if(props.displayComponent){
return props.displayComponent
Copy link
Member

Choose a reason for hiding this comment

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

;

@@ -116,8 +117,12 @@ export default class ScrollNumber extends Component<ScrollNumberProps, ScrollNum
}

renderNumberElement() {
const props = this.props;
Copy link
Member

Choose a reason for hiding this comment

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

- const props = this.props;
+ const { displayComponent } = this.props;

@afc163
Copy link
Member

afc163 commented Sep 12, 2018

Could you add a test case for this feature?

@supra28
Copy link
Contributor Author

supra28 commented Sep 17, 2018

@afc163 Yeah i'll make changes and add a test case this week, as soon as I get some time.

@afc163
Copy link
Member

afc163 commented Oct 12, 2018

@supra28

afc163 added a commit that referenced this pull request Oct 16, 2018
@afc163
Copy link
Member

afc163 commented Oct 16, 2018

Merged in fc6ac42, thx for sending PR

@afc163 afc163 closed this Oct 16, 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.

Typescript error when passing a react node to badge count
4 participants