-
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
chore: Types for FileUploader, FileUploaderButton, FileUploaderDropContainer, FileUploaderItem, FileUploaderSkeleton, Filename, Upload and ButtonSkeleton #13992
Conversation
✅ Deploy Preview for carbon-components-react ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@imp-dance looks like we have some test failures |
@tw15egan Yeah I saw, seems related to this change:
What's the appropriate way to do/propose a breaking change like this? An alternative method could be keeping it as a string but parsing it as an int internally. So should I keep it as a string, or just update the test snapshot? |
@imp-dance if possible, let's keep it a string so it has less of an effect downstream. We could also add |
@tw15egan Sure thing, just pushed a commit that adds both as string & number as valid types, refactored the code to properly parse them and updated the snapshot. |
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.
This looks pretty solid, thanks! I found just a couple minor items to address, let me know what you think
…e/carbon into 12545-add-types-to-fileuploader
@tw15egan This is ready for a re-review @imp-dance There's one minor conflict in button skeleton |
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.
LGTM 👍 ✅ Thanks for contributing! 🎉 💪🏻
@tay1orjones Conflict should be fixed now, merged with latest main 👍 |
Closes #12545
Part of #13229
Part of #13562
Changelog
New
.tsx
:Changed
Filename.tabIndex
changed fromstring
->number
in type and PropType.Additional notes
* Also added types to
Loading
andButtonSkeleton
components. (outside issue scope, but required for issue)* We should probably share a
ButtonKind
type. Could be derived from the constant inprop-types/types.js
.