feat: Add new nav icon for the detours page #2771
Conversation
Coverage of commit
|
| navIcon: React.JSXElementConstructor<HTMLElementProps> | ||
| navIcon: | ||
| | React.JSXElementConstructor<HTMLElementProps> | ||
| | ((props: NavIconProps) => React.JSX.Element) |
There was a problem hiding this comment.
This feels unideal to me, but I'm not sure how to convince typescript that this is okay without doing this.
There was a problem hiding this comment.
IMO, the best way to handle this would be to change navIcon to a ReactNode and stop having the nav supply classes to the content it's trying to render, but the way our CSS is built is probably less friendly to that idea.
assets/src/helpers/navIcons.tsx
Outdated
| export type NavIconProps = ComponentPropsWithoutRef<"span"> | ||
|
|
||
| export const DetourNavIcon = (props: NavIconProps) => ( | ||
| <span {...props}> | ||
| <svg |
There was a problem hiding this comment.
suggestion: I think we should make this function like the icons in our bsIcons, so we'd ideally have the same props and we'd be putting {...props} on the <svg/> and get rid of the <span> inside here. If anything relies on the <span/> being there, I'd instead wrap this icon in some intermediate component.
If it follows our bsicons conventions we could potentially reuse some of that testing code as well that verifies the correct properties are present.
There was a problem hiding this comment.
I had to torture navLinkData.ts (now navLinkData.tsx) a little bit in order to get this to work.
joshlarson
force-pushed
the
jdl/feat/new-detour-nav-icon
branch
from
27e7438 to
3056e3e
Compare
Coverage of commit
|
Before
After
Asana Ticket: https://app.asana.com/0/1205718271156548/1208226797938050/f