Skip to content

[FlatButton] Fix label validation for 0 (number data-type)#4284

Closed
mrdivyansh wants to merge 1 commit intomui:masterfrom
mrdivyansh:flatBtnLabelValidationFix
Closed

[FlatButton] Fix label validation for 0 (number data-type)#4284
mrdivyansh wants to merge 1 commit intomui:masterfrom
mrdivyansh:flatBtnLabelValidationFix

Conversation

@mrdivyansh
Copy link
Copy Markdown

@mrdivyansh mrdivyansh commented May 17, 2016

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s)

FlatButton throw an error when label is zero (numeric data-type). This is special case, so we should handle it in validation function.


function validateLabel(props, propName, componentName) {
if (!props.children && !props.label && !props.icon) {
if (!props.children && (!props.label && props.label !== 0) && !props.icon) {
Copy link
Copy Markdown
Member

@oliviertassinari oliviertassinari May 17, 2016

Choose a reason for hiding this comment

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

I'm wondering if we couldn't

  • use the warning module to remove some of the code in production
  • use React.PropTypes.node to improve this validation method.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@oliviertassinari

  • I don't think any warning is needed here. I am wondering to know, warning message for what ? Component user has not done any mistake if he passed 0 as label.
  • Actually, Label validation doesn't depends on single property. We need to check presence of anyone property out of children, label, and icon. So we can't.

@mrdivyansh
Copy link
Copy Markdown
Author

@mbrookes @oliviertassinari Any update ?

@oliviertassinari
Copy link
Copy Markdown
Member

FlatButton throw an error when label is zero (numeric data-type).

@mrdivyansh Actually, I'm having some doubt about this statement. Have a loot at #4346.

@mrdivyansh
Copy link
Copy Markdown
Author

@oliviertassinari @mbrookes If PR is still not good, please could you do it by own. we need this fix on master branch as soon as possible.

@mbrookes
Copy link
Copy Markdown
Member

mbrookes commented May 30, 2016

Tested in this PR: #4346, specifically here: https://github.com/callemall/material-ui/pull/4346/files#diff-30eabceade84f961fe2a4f955be1f9b1R177

Please open an issue if you believe this is still broken in master.

@mbrookes mbrookes closed this May 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants