-
Notifications
You must be signed in to change notification settings - Fork 96
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(expandable): add type to the button #1982
Conversation
Deploy preview for pf-next ready! Built with commit 50f2d61 |
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.
@christiemolloy I concur. Should we also add {{#if button--IsSubmit}} type="submit"{{/if}}{{#if button--IsReset}} type="reset"{{/if}}
to the button component and update form component button examples?
I agree with @mattnolting - let's add this to the button component. And should we add |
Updated @srambach and @mattnolting |
@@ -5,6 +5,9 @@ Always add a modifier class to add color to the button. | |||
## Button vs link | |||
Semantic buttons and links are important for usability as well as accessibility. Using an `a` instead of a `button` element to perform user initiated actions should be avoided, unless absolutely necessary. | |||
|
|||
## Button Types | |||
The default type for a button is button, which must be set to avoid having a button in a form incorrectly default to type "submit". If a button is used in a submit form, the type should be set to "submit". If a button is used to reset a form, the type should be set to "reset". |
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.
Do we want to include this statement here? The type
attribute is documented on MDN as standard html, and not specific to patternfly. And the statement about what the default is is specific to our handlebar, but is not something our consumers would care about.
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.
Agreed I honestly wasn't too sure about putting it in but wanted to see what others thought.
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.
These updates look good! I just have one tiny nit about the documentation section that was added, but the rest looks great.
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.
🎉 This PR is included in version 2.15.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
close #1980
I think the problem was that there was no "type" set on the toggle button in the expandable component, so when it was added to a form the type defaulted to submit.
I think its safe to add the type as button to this component because I couldnt see it being a reset/submit type.
Maybe we should also evaluate the variations of the button component.