-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[UI Framework] Rename "type" property to "buttonType" #11878
Conversation
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.
Awesome! Thanks for doing this man. Just had a couple suggestions.
data-test-subj="confirmModalConfirmButton" | ||
type="primary" |
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 like we'll need to update confirm_modal.js
, which creates instances of KuiButton
.
BUTTON_TYPES.forEach(buttonType => { | ||
describe(buttonType, () => { | ||
test(`renders the ${buttonType} class`, () => { | ||
const $button = render(<KuiButton buttonType={buttonType} />); |
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.
We'll need to update the test above this one to be:
test('HTML attributes are rendered', () => {
const $button = render(
<KuiButton
aria-label="aria label"
className="testClass1 testClass2"
data-test-subj="test subject string"
type="submit"
disabled
/>
);
expect($button)
.toMatchSnapshot();
});
This will allow us to test that specifying the prop type="submit"
renders as the HTML attribute type="submit"
.
We'll need to do this with the KuiLinkButton and KuiSubmitButton tests. too.
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 made one mistake in my original feedback. Other than that, LGTM!
@@ -24,6 +24,7 @@ describe('KuiSubmitButton', () => { | |||
aria-label="aria label" | |||
className="testClass1 testClass2" | |||
data-test-subj="test subject string" | |||
type="submit" |
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'm sorry, I forgot that this component will always apply the type="submit"
attribute. So we can remove this from this test.
@cjcenizal was this supposed to go to 5.x too? #12050 was tagged with 5.x so I backported it, but this isn't there |
If so, when you backport them will you also revert dbd3c83? |
* change type property to buttonType * Update the tests to assert that the type HTML attribute is supported * remove type=submit from KuiSubmitButton since component automatically sets type
* [UI Framework] Rename "type" property to "buttonType" (#11878) * change type property to buttonType * Update the tests to assert that the type HTML attribute is supported * remove type=submit from KuiSubmitButton since component automatically sets type * Revert "Revert "Fix use of KuiButtons in Clone Modal."" This reverts commit dbd3c83.
* change type property to buttonType * Update the tests to assert that the type HTML attribute is supported * remove type=submit from KuiSubmitButton since component automatically sets type
Fixes #11871
Renames
type
property tobuttonType
.