chore: added types to TreeNode & TreeView #17038
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Also looks like this is the last checkbox @ #12513 - which I guess means the issue should either be closed or be updated with new items 🚀 |
There was a problem hiding this comment.
Hi @imp-dance! 👋
Great work on the PR, it's looking good overall. I have one small suggestion:
Could you please add comments to the remaining props in the interface? This helps improve the intellisense for the typings, making it easier for developers to understand and use the component.
Thanks for your attention to detail! 🙌
Done! 🚀 |
|
Actually, hold on a bit - I'll update the index file as well, and make sure that |
|
@Gururajj77 Done 👍 I'd appreciate another quick look over the changes when you can 😎 |
| @@ -310,3 +388,6 @@ TreeView.propTypes = { | |||
| */ | |||
| size: PropTypes.oneOf(['xs', 'sm']), | |||
| }; | |||
|
|
|||
| TreeView.TreeNode = TreeNode; | |||
There was a problem hiding this comment.
Would you mind adding a test to ensure <TreeView.TreeNode> works?
There was a problem hiding this comment.
Done, I just copy/pasted should render children as expected into should render children as expected when using dot syntax and refactored it to use <TreeView.TreeNode> instead of <TreeNode>.
| // These are provided by the parent TreeView component | ||
| const depth = propDepth as number; | ||
| const selected = propSelected as (string | number)[]; |
There was a problem hiding this comment.
I have another proposal.
Since the component internals expect that these should always be defined, should we do something like this to provide a better error for consumers that try to mount TreeNode outside of TreeView?
if (depth === undefined || selected === undefined){
throw new Error("It looks like you are trying to mount a TreeNode outside of a TreeView. To do this, ensure that the 'selected' and 'depth' props are provided.");
}This would also mean that we wouldn't have to cast the types manually either.
| // @ts-ignore - will always be false according to prop types | ||
| [`${prefix}--tree--${size}`]: size !== 'default', | ||
| }); |
There was a problem hiding this comment.
I also found this line of code a bit strange, considering size should only be xs or sm (according to types).
6a32844
Closes #13581
Relates to #12513
Changelog
New
Changed
undefined, notnullTesting / Reviewing
Did a tiny test, but it'd be useful if someone with intimate knowledge of how the component internals work could take a look.
Some of the props (
depth,selected) seem to be meant for only internal use, so I've typed them as optional (and ensured that the JSDoc comment is there) - but made sure to cast them as non nullable inside the component - assuming the parent ensures that they are given.I also had to use
// @ts-ignorein a couple of spots, namely in the PropTypes - since theCarbonIconTypedoes not seem to matchPropTypes.oneOfType([PropTypes.func, PropTypes.object]), and(string | number)[]does not matchPropTypes.arrayOf( PropTypes.oneOfType([PropTypes.string, PropTypes.number]) ),. If you know of any way we could drop the use of ts ignore here and fix the types or proptypes, that would be great.