Conversation
maxn990
left a comment
There was a problem hiding this comment.
I like the overall approach! Just a few quick notes on documentation and ideas to make it less permissive
maxn990
left a comment
There was a problem hiding this comment.
Can you clarify for me why exactly we need to wrap everything in <Text>? This might be my bad, I may have led you down the wrong path in my last review. I'm happy to help take these off myself if we need to, I feel bad if I wasted your time 😭
But here's what I'm thinking: if the reason for the is because we're only accepting react elements as child components, then maybe we could do children?: JSX.Element | JSX.Element[] | string instead.
I think we should be less permissive than any just because essentially disabling type checking sort of ruins the point of ts. The extremely tedious but probably most correct thing to do would be to go through each component and manually set what it can accept as a child (ie MenuItemProps takes a string and CardBodyProps takes a react element, or even a specific react element. Unfortunately, this would mean that every time we add a new component we'd also have to add it to this file. So if we can find a solution that isn't overly permissive but also works for all components that could be good.
Truthfully I'm not sure there's a very elegant solution to this, so if we need to be overly permissive maybe that's that. I hope chakra releases a fix for this soon.
| @@ -0,0 +1,81 @@ | |||
| /* eslint-disable @typescript-eslint/no-empty-object-type */ | |||
There was a problem hiding this comment.
Lol sorry this is really picky but can we fix the filename charka-ui.d.ts to chakra.u.d.ts.
ℹ️ Issue
Closes #140
📝 Description
This PR tackled the issue with components that we encountered during our Chakra v3 migration that made it so we could not use our linter anymore.
As I looked more into it, it seems this root cause is a common one that many developors who use Chakra v3 face. The issue was ultimately that several components have been made to not have an "asChild" prop attached to it, making it unable to be nested in other components, which we needed in the case of almost all of our pages. While there were various solutions that seemed possible, the one I thought would be the easiest would be to override the module by allowing specified props to accept children.
I created a new file that extended an interface to allow each component we specified to be a child, accept children, have polymorphic component props (basically render different things such as icons, links, etc.) and anything else they may need. These permissions are very loose, but seem to be necessary here.
With this, our linter now works again.
✔️ Verification
Smoke-tested all of the frontend
🏕️ (Optional) Future Work / Notes
Any new components that cannot accept children may need to be added to the chakra-ui.d.ts file