refactor: add typescript types to TreeNode and TreeView #14775
Conversation
|
All contributors have signed the DCO. |
|
I have read the DCO document and I hereby sign the DCO. |
✅ 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. |
cesardlinx
force-pushed
the
issue_13581
branch
from
618190d to
0613935
Compare
|
@cesardlinx Hey David, do you mind fixing the merge conflicts - we'll re-review and get it merged! |
|
Hey @andreancardona sure thing! I'll fix it 🙂 |
|
Hey @andreancardona I have fixed the merge conflict, please let me know if you need anything else |
| <div className={`${prefix}--tree-node__label`} ref={currentNodeLabel}> | ||
| {/* https://github.com/carbon-design-system/carbon/pull/6008#issuecomment-675738670 */} | ||
| {/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */} | ||
| <span | ||
| className={`${prefix}--tree-parent-node__toggle`} | ||
| disabled={disabled} | ||
| // disabled={disabled} |
There was a problem hiding this comment.
I think this needs to remain
There was a problem hiding this comment.
If I add it back I get this error
Basically what it is trying to say is that span element doesn't have a disabled prop. And it makes sense because only html elements such as button, input, select, textarea have the disabled attribute
There was a problem hiding this comment.
Oh my bad, yeah let's just remove it then
| @@ -288,7 +464,7 @@ TreeNode.propTypes = { | |||
| * Optional prop to allow each node to have an associated icon. | |||
| * Can be a React component class | |||
| */ | |||
| renderIcon: PropTypes.oneOfType([PropTypes.func, PropTypes.object]), | |||
There was a problem hiding this comment.
Narrowing proptypes is considered a breaking change. We'll need to leave this as is for now.
There was a problem hiding this comment.
The problem was that by leaving it as how it was I get this error:
It appears to be a conflict between TypeScript and PropTypes in this section. The closest issue I found resembling this problem is documented here: DefinitelyTyped/DefinitelyTyped#28015
| @@ -148,6 +225,7 @@ export default function TreeView({ | |||
| } | |||
| } | |||
| } | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
What error is being suppressed here?
There was a problem hiding this comment.
Here I'm suppressing the same error as before
Co-authored-by: Taylor Jones <[email protected]>
|
Thank you @tay1orjones for the valuable feedback. I've addressed some of the issues you highlighted, and also I've provided responses to your observations. |
|
Hey there, sorry for the delay in response here. From what I can tell there's two pieces we need to resolve here
|
|
Hey @tay1orjones. Sorry, work has kept me busy lately and I almost forgot about this PR. I'll check it out during the weekend and I'll keep you posted. |
|
Hey @tay1orjones, I've resolved the issues you pointed out. Let me know if there's anything else I can help with. |
|
Hi @cesardlinx, Apologies for the delay. Could you please resolve the merge conflicts so that we can proceed? Thank you. |
|
This was put together before the addition of the TreeView controllable api, which after resolving the merge conflicts there's many tests failing. We're going to close this one in favor of opening a new one based on these changes but with the controllable api taken into consideration. Sorry for the delay here |
Closes #13581
Typescript annotations added to TreeNode and TreeView
Changelog
Changed
Removed
Testing / Reviewing