-
Notifications
You must be signed in to change notification settings - Fork 637
Add browser tests for polymorphic as
prop behavior in styled-react components
#6911
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 browser tests for polymorphic as
prop behavior in styled-react components
#6911
Conversation
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
describe('@primer/react', () => { | ||
test('ActionList supports `sx` prop', () => { | ||
render(<ActionList data-testid="component" sx={{background: 'red'}} />) | ||
render(<ActionList as="ul" data-testid="component" sx={{background: 'red'}} variant="inset" />) |
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 caught my eye, why do we need to explicitly set ActionList as="ul"?
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.
Hi @siddharthkp, thanks for the review! Do you mean that we should use as="div"
instead of the default ul
for ActionList
?
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.
No sorry, I meant why do we need to pass it any as
prop? Does it no longer render a <ul>
by default?
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.
The as
prop in this test is specifically to catch the polymorphic
prop forwarding issue that the pattern in PR #6910 fixes, when you wrap a component with styled()
, the as
prop can get consumed by styled-components
instead of being forwarded to the wrapped component, which causes component specific props to fail to be consumed correctly. That's why we add both as
and variant
here to test that variant
is correctly passed.
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.
Oh lol, my bad! That is literally what you are testing 😅
Do you mean that we should use as="div" instead of the default ul for ActionList?
This is a very good idea in that case!
…s-for-polymophic-components
…s-for-polymophic-components
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.
✨
Adds browser tests to verify that
styled-react
components correctly handle the polymorphicas
prop. Ensures styled-components wrappers maintain component behavior when rendering and provides test coverage for component-specific props preservation.Current State
Tests currently fail for several components, reproducing the existing
as
prop forwarding issues in styled-react wrappers. PR #6910 should fix these test failures.