-
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
Add type attribute to tag remove button #551
Conversation
Thanks for your interest in palantir/blueprint, @devjunkORG! 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.
nice catch!
9196eb1
to
fdf315f
Compare
@@ -35,7 +35,8 @@ export class Tag extends React.Component<ITagProps, {}> { | |||
return ( | |||
<span {...removeNonHTMLProps(this.props)} className={tagClasses}> | |||
{this.props.children} | |||
{isFunction(onRemove) ? <button className={Classes.TAG_REMOVE} onClick={onRemove} /> : null} | |||
{isFunction(onRemove) ? <button type="button" className={Classes.TAG_REMOVE} onClick={onRemove} /> | |||
: null} |
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.
pull this out to a local variable or bring back to one line.
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.
I was doing just that. Also, I'm sorry, I added PR metadata to the commit message before I should have.
c547bd3
to
256dd98
Compare
comply with linting rules PR: palantir#551 refactor button, move to variable
256dd98
to
b5e4a6c
Compare
|
||
return ( | ||
<span {...removeNonHTMLProps(this.props)} className={tagClasses}> | ||
{this.props.children} | ||
{isFunction(onRemove) ? <button className={Classes.TAG_REMOVE} onClick={onRemove} /> : null} | ||
{isFunction(onRemove) ? button : null} |
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.
i meant pull the whole statement out. no need to create the object when the function is false. this should say just {button}
also please use undefined
instead of null
.
b5e4a6c
to
b61ea90
Compare
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.
thanks! for the record, if you're gonna fix something immediately then there's no need to file an issue. just submit the fix directly!
will keep that in mind, thanks! |
Fixes #550
Checklist
Changes proposed in this pull request:
Add
type
attribute to remove buttons on Tag components, since not having type="button" submits forms that contain the element on some browsers.