-
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
refactor(containedlist): added typescript types to containedlist #16115
refactor(containedlist): added typescript types to containedlist #16115
Conversation
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Just a couple small things I noticed
/** | ||
* A label describing the contained list. | ||
*/ | ||
label: ReactNode; |
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 accepts a string too, right?
label: ReactNode; | |
label: string | ReactNode; |
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.
Yes, I overlooked it, will make the changes.
import PropTypes from 'prop-types'; | ||
import classNames from 'classnames'; | ||
import { LayoutConstraint } from '../Layout'; | ||
import { useId } from '../../internal/useId'; | ||
import { usePrefix } from '../../internal/usePrefix'; | ||
|
||
const variants = ['on-page', 'disclosed']; | ||
const variants: ('on-page' | 'disclosed')[] = ['on-page', 'disclosed']; |
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.
Is there a way to use this root definition for both the proptypes and the TS interface? Something like this maybe
const variants = ['on-page', 'disclosed'] as const;
// I think this will be typed as 'on-page' | 'disclosed' because the typeof operator returns the inferred type?
type Variants = typeof variants[number];
// ...then below in the interface:
kind?: Variants;
const isActionSearch = | ||
React.isValidElement(action) && | ||
(typeof action.type === 'string' | ||
? ['Search', 'ExpandableSearch'].includes(action.type) | ||
: typeof action.type === 'function' && | ||
'displayName' in action.type && | ||
(action.type.displayName === 'Search' || | ||
action.type.displayName === 'ExpandableSearch')); |
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.
Is there any way to simplify this? It's really dense and hard to read. Is there another spot in the codebase we validate/check displayName/type?
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.
I will to simplify this and make it more readable.
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 👍 ✅
b6e8253
Closes #16019
Added typescript types to ContainedList component
Changelog
New
Changed
Testing / Reviewing
This new PR holds the review comments from #16047
No new observations on testing.