-
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: add types to ComboButton
#16038
feat: add types to ComboButton
#16038
Conversation
Convert `ComboButton` to Typescript and export newly added interface `ComboButtonProps`.
✅ 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. |
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 apologize for the diff. I did execute git mv
instead of directly renaming the file, but after the pre-commit hooks ran it updated the diff 🫠.
In an effort to make the review easier, I documented my changes below.
/** | ||
* Specify the size of the buttons and menu. | ||
*/ | ||
size?: 'sm' | 'md' | 'lg'; |
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 size
is passed to Button
, IconButton
, and Menu
, I typed it statically as each component accepts different, additional values.
For example, Menu
also accepts xs
, which Button
and IconButton
do not accept. Conversely, the latter components accept xl
and 2xl
.
const containerRef = useRef<HTMLDivElement>(null); | ||
const menuRef = useRef<React.ComponentRef<typeof Menu>>(null); |
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.
Added types for containerRef
and menuRef
.
} | ||
} | ||
|
||
function handlePrimaryActionClick(e: React.MouseEvent<HTMLButtonElement>) { |
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.
Added type for the event (e
) argument.
if (menuRef.current) { | ||
menuRef.current.style.inlineSize = `${width}px`; | ||
menuRef.current.style.minInlineSize = `${width}px`; | ||
|
||
if (menuAlignment !== 'bottom' && menuAlignment !== 'top') { | ||
menuRef.current.style.inlineSize = `fit-content`; | ||
} | ||
} |
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.
Added a null-check for menuRef.current
.
disabled: PropTypes.bool, | ||
|
||
/** | ||
* Provide the label to be rendered on the primary action button. |
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.
Fixed a typo: "renderd" → "rendered".
return defaultTranslations[messageId]; | ||
} | ||
|
||
interface ComboButtonProps { |
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.
New types for component props.
Where possible, I pulled the types dynamically from the components that consume these props e.g., children
is passed to Menu.children
.
'carbon.combo-button.additional-actions': 'Additional actions', | ||
}; | ||
|
||
function defaultTranslateWithId(messageId: 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.
Added type for messageId
.
translateWithId?: (id: string) => string; | ||
} | ||
|
||
const ComboButton = React.forwardRef<HTMLDivElement, ComboButtonProps>( |
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.
Added types for ComboButton
and its props
.
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 great, thanks so much!
daddb69
Convert `ComboButton` to Typescript and export newly added interface `ComboButtonProps`.
Closes #15178
Convert
ComboButton
to Typescript and export newly added interfaceComboButtonProps
.Changelog
New
ComboButton/index.js
→ComboButton/index.tsx
.ComboButtonProps
.Changed
ComboButton.propTypes.label
description.Testing / Reviewing
PropTypes
.