Migrate ButtonBase component to TypeScript #18950
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
I have read the CLA Document and I hereby sign the CLA |
georgewrmarshall
added
the
team-design-system
There was a problem hiding this comment.
Hey @makoto, thanks for your contribution. This is looking really great! I've made some suggestions that weren't outlined in the original issue so my apologies. I'll make sure other issues have been updated with the enum conventions.
ui/components/component-library/button-base/button-base.types.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * The polymorphic `as` prop allows you to change the root HTML element of the Button component between `button` and `a` tag | ||
| */ | ||
| as?: ValidTag; |
There was a problem hiding this comment.
Wondering if we should just limit this to button and a link element cc @garrettbear
type ButtonOrAnchorTag = 'button' | 'a';
ui/components/component-library/button-base/button-base.types.ts
Outdated
Show resolved
Hide resolved
ui/components/component-library/button-base/button-base.stories.tsx
Outdated
Show resolved
Hide resolved
ui/components/component-library/button-base/button-base.stories.tsx
Outdated
Show resolved
Hide resolved
|
Hi @georgewrmarshall, thanks for the all suggestions! I think I've implemented them correctly. Let me know if there is anything else. |
There was a problem hiding this comment.
Hey @thebinij, Looking really good. I left a couple more suggestions thanks!
| SM = 'sm', | ||
| MD = 'md', | ||
| LG = 'lg', |
There was a problem hiding this comment.
Our enum conventions should use PascalCase for keys too! Sorry some are a little mixed.
| SM = 'sm', | |
| MD = 'md', | |
| LG = 'lg', | |
| Sm = 'sm', | |
| Md = 'md', | |
| Lg = 'lg', |
ui/components/ui/box/box.d.ts
Outdated
| @@ -290,7 +290,7 @@ export interface BoxProps extends React.HTMLAttributes<HTMLElement> { | |||
| * Use TEXT_ALIGN const from '../../../helpers/constants/design-system'; | |||
| * Accepts responsive props in the form of an array. | |||
| */ | |||
| textAlign?: BoxTextAlign | BoxTextAlignArray; | |||
| textAlign?: BoxTextAlign | BoxTextAlignArray | TextAlign; | |||
There was a problem hiding this comment.
Ahh I think Box still uses the deprecated version of TextAlign. Would you be able to update it to use the TextAlign enum and remove instances of TEXT_ALIGN Similar to other enums in the file
Create new type
export type TextAlignArray = [
TextAlign,
TextAlign?,
TextAlign?,
TextAlign?,
];
Then you should be able to remove the old types
export type BoxTextAlign = (typeof TEXT_ALIGN)[keyof typeof TEXT_ALIGN] | null;
export type BoxTextAlignArray = [
BoxTextAlign,
BoxTextAlign?,
BoxTextAlign?,
BoxTextAlign?,
];
| textAlign?: BoxTextAlign | BoxTextAlignArray | TextAlign; | |
| textAlign?: TextAlign | TextAlignArray; |
yarn.lock
Outdated
| checksum: bc8ab55cd194e240152946b54bfaff7456180cc018674fc7ed134f4f502192405f6643f422feaa0a5e7cc02b5bac564cfac7771ac6d29f5d129482fcfe335ba1 | ||
| version: 1.0.30001482 | ||
| resolution: "caniuse-lite@npm:1.0.30001482" | ||
| checksum: a5f7681c860a29736f29350ebd81041c40b6aa7b2f94c50ed27284a0507e46dc79536dcfc05432504cfc80a0bf2070e4ad6fa704a9c0f3f32d47bed9059e98c2 | ||
| languageName: node | ||
| linkType: hard | ||
|
|
There was a problem hiding this comment.
Hmm shouldn't be any changes to this file can you please revert
There was a problem hiding this comment.
Oh I didn't know . Okay sure.
thebinij
force-pushed
the
#18886-Migrate-ButtonBase
branch
from
3e6362e
to
124678e
Compare
Explanation
Closes #18886
Manual Testing Steps
No functional changes
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.