-
Notifications
You must be signed in to change notification settings - Fork 58
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: forwardRef from SideNavigationLink component #1123
fix: forwardRef from SideNavigationLink component #1123
Conversation
befd701
to
fcbcf77
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.
It looks like most of the changes need to be reverted from the previous PR: https://github.com/canonical/react-components/pull/1118/files as removing the generic types has broken then ability to handle props when passing component
.
For example when using this with a Link
from React Router it would give an error for the to
prop here as it can't infer the props of the Link
component:
<SideNavigationLink label="docs" component={Link} to="/docs" />
Unfortunately forwardRef
does not support components with generic types, so instead you have to manually pass a ref. We do that in a few places e.g.
forwardRef?: React.Ref<HTMLDivElement> | null; |
This issue could be caught in the future if we change this test:
react-components/src/components/SideNavigation/SideNavigationLink/SideNavigationLink.test.tsx
Line 34 in f60d6bc
it("can use a custom link component", () => { |
To:
it("can use a custom link component", () => {
const Link = ({ to }: { to: () => void }) => (
<button onClick={to}>link</button>
);
const label = "Test content";
render(<SideNavigationLink label={label} component={Link} to={jest.fn()} />);
expect(screen.getByRole("button", { name: label })).toHaveClass(
"p-side-navigation__link",
);
});
fcbcf77
to
a0e8fd4
Compare
Thank you Huw for pointing that out! I've updated the code and the test like you suggested. Can you please take a look again? |
a0e8fd4
to
9d67f7e
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.
This change also needs to be reverted https://github.com/canonical/react-components/pull/1118/files#diff-011b080bc51b47c868f250b3a34619b5f41e4c16c8e39a6636ccc719b6da8bd6L40
src/components/SideNavigation/SideNavigationLink/SideNavigationLink.tsx
Outdated
Show resolved
Hide resolved
4ee5aac
to
7ed1fa0
Compare
7ed1fa0
to
3b76473
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 this fix!
🎉 This PR is included in version 1.2.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Done
Percy steps