-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fixes: helperText no longer accepting React nodes #2280
Conversation
Thanks for your interest in palantir/blueprint, @will-stone! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great 👍 ashamed this got in in the first place
@@ -42,7 +42,7 @@ export class Label extends React.PureComponent<ILabelProps, {}> { | |||
return ( | |||
<label {...htmlProps} className={rootClasses}> | |||
{text} | |||
{helperText && <span className={classNames(Classes.TEXT_MUTED)}>{" " + helperText}</span>} | |||
{helperText && <span className={classNames(Classes.TEXT_MUTED)}>{" "}{helperText}</span>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while we're at it, can you remove the classNames()
call? not necessary for one class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@will-stone i went ahead and made this change here. thanks!
@@ -42,7 +42,7 @@ export class Label extends React.PureComponent<ILabelProps, {}> { | |||
return ( | |||
<label {...htmlProps} className={rootClasses}> | |||
{text} | |||
{helperText && <span className={classNames(Classes.TEXT_MUTED)}>{" " + helperText}</span>} | |||
{helperText && <span className={Classes.TEXT_MUTED}>{" "}{helperText}</span>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tslint/prettier is complaining about this line:
Replace {"·"}
with ·
Fixes #2279
Changes proposed in this pull request:
The current implementation of helperText on Label only allows for text, this fixes the regression and allows for React nodes again.
Reviewers should focus on:
Adding React nodes as helperText