-
Notifications
You must be signed in to change notification settings - Fork 86
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
chore(button-bar): convert enzyme test to rtl #6737
Conversation
expect(button).toHaveStyle("font-size: var(--fontSizes100)"); | ||
expect(button).toHaveStyle("min-height: 40px"); |
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.
suggestion(non-blocking): toHaveStyle
accepts an object so you could put these into one assertion. I'm happy with either way. It's just whichever way we decide we want to do it going forward.
expect(button).toHaveStyle("font-size: var(--fontSizes100)"); | |
expect(button).toHaveStyle("min-height: 40px"); | |
expect(button).toHaveStyle({ | |
fontSize: "var(--fontSizes100)", | |
minHeight: "40px", | |
}); |
const ariaLabelValue = | ||
ariaLabel || | ||
(internalRef?.querySelector( | ||
"[data-component='icon']" | ||
) as Element)?.getAttribute("type") || | ||
""; |
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.
praise: nice one for removing this decoupling 👍
}); | ||
}); | ||
|
||
it("should render the Buttons with the 'iconPosition' prop set to 'before'", () => { |
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.
suggestion(non-blocking): how about we amend a few of test titles to make the intended behavior clearer? This way, if something breaks and a test fails, it will be easier for us to understand what the component is supposed to do.
For example:
it("renders icon with right margin when iconPosition prop is 'before'");
it("does not render Buttons with full width when fullWidth prop is false");
I appreciate that most of the tests are checking trivial CSS rules though, so I'll leave this up to you. What are your thoughts?
🎉 This PR is included in version 136.0.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Proposed behaviour
Converts enzyme tests to rtl.
Current behaviour
Current tests are written in enzyme.
Checklist
d.ts
file added or updated if requiredQA
Additional context
Testing instructions