fix: logo prop refactor to component based#85
Conversation
| it("should render a custom component as the logo", () => { | ||
| const CustomLogo = ({ title }: { title: string }) => ( | ||
| <svg role="img" aria-label={title}> | ||
| <title>{title}</title> | ||
| </svg> | ||
| ); | ||
| const { getByRole } = render(<NavBar logo={{ image: <CustomLogo title="Custom logo" /> }} />); | ||
|
|
||
| expect(getByRole("img", { name: "Custom logo" })).toBeInTheDocument(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
What I understand that there is no alt message support if user provided none. At least we could define default value for alt prop to show some value if there is a problem while rendering custom image or logo.
There was a problem hiding this comment.
With the new commit, we've moved to a flexible approach: logoSlot: ReactElement
This allows users to pass any component (img, Image, SVG, etc.)
with full control over attributes like alt etc.
This resolves the concern about alt text support.
| imgPath: string; | ||
| alt?: string; | ||
| image: ReactElement; | ||
| href?: string; |
There was a problem hiding this comment.
Actually this change got me stop reviewing any further. This is an additional feature and we dont need to drop supporting previous props. If you do this way, the apps using this props will collapse.
So, I would keep the current props as it is and still render an , put an additional
logoSlot: ReactElement prop to the Navbar itself. if logo is given, it should render that. If only logoSlot is given, then render that.
3 participants
Description
NavBar logo prop refactored to accept React components instead of image paths.
Changes:
imgPath: stringpropimage: ReactElementfor flexible component rendering<img>,<Image>,<NextImage>, and other custom componentsType of Change
Checklist
How to Test