-
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
feat: Type pagination #15399
feat: Type pagination #15399
Conversation
9966f0f
to
0d615c5
Compare
|
||
export interface PaginationPageSize { | ||
text: string; | ||
value: string; |
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.
Completely appreciate this is still a draft, but just to flag in the case it's helpful, based on this (and its display in the docs);
carbon/packages/react/src/components/Pagination/Pagination.tsx
Lines 508 to 516 in d30eb7c
pageSizes: PropTypes.oneOfType([ | |
PropTypes.arrayOf(PropTypes.number), | |
PropTypes.arrayOf( | |
PropTypes.shape({ | |
text: PropTypes.string, | |
value: PropTypes.number, | |
}) | |
), | |
]).isRequired, |
This might need to be a number
value: string; | |
value: number; |
✅ 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. |
pageSizes: PropTypes.oneOfType([ | ||
PropTypes.arrayOf(PropTypes.number), | ||
PropTypes.arrayOf(PropTypes.number.isRequired), | ||
PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
text: PropTypes.text, | ||
value: PropTypes.number, | ||
}) | ||
text: PropTypes.string.isRequired, | ||
value: PropTypes.number.isRequired, | ||
}).isRequired |
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.
Since I updated text -> string, and needed to add some isRequired to fields to prevent typescript errors, I needed to update the public api snapshot accordingly. Not sure if this will cause this to needed to go out in a major change.
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.
Cool. I don't think it will a problem!
Hopefully this can be merged soon, AFAIK it's the last Carbon problem preventing us from upgrading a bunch of our code to Carbon 11. |
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.
looks good to me - thank you for doing this!
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 the contribution!
pageSizes: PropTypes.oneOfType([ | ||
PropTypes.arrayOf(PropTypes.number), | ||
PropTypes.arrayOf(PropTypes.number.isRequired), | ||
PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
text: PropTypes.text, | ||
value: PropTypes.number, | ||
}) | ||
text: PropTypes.string.isRequired, | ||
value: PropTypes.number.isRequired, | ||
}).isRequired |
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.
Cool. I don't think it will a problem!
9346f3e
* feat: add types to Pagination * refactor: update pageSizes proptype to correctly match interface * test: update public snapshot since proptypes changed
Closes #13566
adds TypeScript types to Pagination and PaginationSkeleton. Depends on #15057
Changelog
Changed
Removed
Testing / Reviewing
Verify that unit test run, verify that storybook works as intended