feat: add types to Copy and CopyButton #15213
Conversation
| // TODO: remove these two lines when IconButton is properly typed, revert TypedIconButton component to IconButton | ||
| type IconButtonType = React.ButtonHTMLAttributes<HTMLButtonElement> & { | ||
| closeOnActivation?: boolean; | ||
| align: string; | ||
| label: string | undefined; | ||
| }; | ||
| const TypedIconButton = IconButton as React.FC<IconButtonType>; | ||
|
|
||
| return ( | ||
| <IconButton | ||
| <TypedIconButton |
There was a problem hiding this comment.
IconButton is not currently typed and as a result causes errors without defining a type for it. #15057 is currently in PR and adds types for IconButton allowing us to remove these lines then. As a result I can either wait on #15057 and remove these lines, or continue with this approach and clean up this todo in the future.
There was a problem hiding this comment.
also, should i change the extends for the Copy component as well to extend Carbon button rather than the native button
There was a problem hiding this comment.
@Tresau-IBM, sorry for the delay on this one. Now that #15057 has been merged, can we use IconButton?
✅ 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. |
| import Copy from '../Copy'; | ||
| import { LayoutConstraint } from '../Layout'; | ||
| import { usePrefix } from '../../internal/usePrefix'; | ||
| import { noopFn } from '../../internal/noopFn'; | ||
|
|
||
| export interface CopyButtonProps extends ButtonProps<'button'> { |
There was a problem hiding this comment.
I have changed this to extend Carbon's ButtonProps rather than the native button. I have done this because CodeSnippet passes a size prop to CopyButton, which gets used by Button down the line. Should i also change the Copy type to extend ButtonProps? IS this even the best way to handle the issue, or the correct way to extend the Carbon button.
|
@Tresau-IBM now that the types for |
sorry for the delay, was unavailable, will update this shortly :) |
|
@Tresau-IBM are you able to resolve the conflicts? Thank you! |
|
@tw15egan @andreancardona, the conflicts have been resolved and the workaround for IconButton has been removed. |
tay1orjones
added
status: ready to merge 🎉
and removed
status: ready for review 👀
status: one more review 👀
labels
e443e39
|
Hey there! v11.53.0 was just released that references this issue/PR. |
* feat: add types to Copy and CopyButton * refactor: update index files to ts * refactor: extend Carbon Button rather than native button * refactor: use IconButtun instead of manually created TypedButton --------- Co-authored-by: Andrea N. Cardona <[email protected]> Co-authored-by: TJ Egan <[email protected]>
Closes #13551
Closes #13227
Add TypeScript types for Copy and CopyButton
Changelog
New
Changed
Removed
Testing / Reviewing
Verify that the unit tests run and pass.
Verify that the Storybook for CopyButton works as intended.