-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add typescript types to combo box #12873
Add typescript types to combo box #12873
Conversation
DCO Assistant Lite bot All contributors have signed the DCO. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
I have read the DCO document and I hereby sign the DCO. |
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for contributing this! One minor ask - instead of renaming the .js
file to .tsx
, could you actually rename it using git?
git mv packages/react/src/components/ComboBox/ComboBox.js packages/react/src/components/ComboBox/ComboBox.tsx
This way the diff will clearly show the file extension change but also the proper unified diff showing your changes within file.
Right now it's hard to tell what you've added to the file vs was was in it to begin with.
Also we should wait to merge this until #12787 is in
Thanks, didn't realize that was the better way of renaming it. It should look better now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sierrawetmore Awesome yeah that looks great! #12787 is in - I think it caused some merge conflicts here
Bump @sierrawetmore |
This is on my to-do list in the next couple days. Also waiting to see where we land on the generic types conversation. |
50bd881
to
911d66f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest blocker here I think is the props being destructured out of ...rest
. Also a few questions below
055f1c1
to
17d4c74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality hasn't changed, no failing tests / a11y violations. LGTM 👍🏻 ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! 🏆
17d4c74
to
b15f51e
Compare
Closes #12538
This PR converts ComboBox.js to typescript.
Changelog
Changed
Testing / Reviewing
These changes can be tested and verified by running a fresh yarn, yarn build and yarn storybook. Verify that the behavior in the ComboBox story remains unchanged