-
Notifications
You must be signed in to change notification settings - Fork 202
[BpkLabel] Migrate component to TypeScript #4110
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
Conversation
Co-authored-by: Faye-Xiao <20058385+Faye-Xiao@users.noreply.github.com>
Co-authored-by: Faye-Xiao <20058385+Faye-Xiao@users.noreply.github.com>
|
Visit https://backpack.github.io/storybook-prs/4110 to see this build running in a browser. |
|
Hi Copilot, It has some issues in the build as follows:
Fix these issues for me |
- Add @ts-expect-error for untyped bpk-storybook-utils import - Remove unused @ts-expect-error directives from files importing BpkLabel (now typed) - Update BpkLabel valid prop type to accept null (matching original Flow type) Co-authored-by: Faye-Xiao <20058385+Faye-Xiao@users.noreply.github.com>
Fixed all the TypeScript build errors in commit 5d84a61:
|
|
Visit https://backpack.github.io/storybook-prs/4110 to see this build running in a browser. |
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.
Pull request overview
This PR migrates the BpkLabel component from Flow to TypeScript, enhancing type safety and modernizing the test suite. The migration replaces PropTypes with native TypeScript types and converts snapshot tests to explicit assertions.
Key changes:
- Converted BpkLabel component and all related files to TypeScript with proper type definitions
- Replaced 7 snapshot tests with explicit Jest assertions using
@testing-library/jest-dom - Removed
@ts-expect-errordirectives from dependent components now that BpkLabel is typed
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
packages/bpk-component-label/src/BpkLabel.tsx |
Migrated component from Flow to TypeScript, removed PropTypes, added ComponentPropsWithoutRef<'label'> for HTML attribute typing |
packages/bpk-component-label/src/BpkLabel-test.tsx |
Replaced snapshot tests with explicit Jest assertions |
packages/bpk-component-label/src/accessibility-test.tsx |
Removed Flow type annotation |
packages/bpk-component-label/index.ts |
Updated exports to TypeScript syntax |
packages/bpk-component-label/README.md |
Changed code example from js to tsx |
packages/bpk-component-nudger/src/BpkNudger.tsx |
Removed @ts-expect-error for now-typed BpkLabel import |
packages/bpk-component-fieldset/src/BpkFieldset.tsx |
Removed @ts-expect-error for now-typed BpkLabel import |
examples/bpk-component-textarea/examples.tsx |
Removed @ts-expect-error for now-typed BpkLabel import |
examples/bpk-component-label/examples.tsx |
Removed Flow annotation, added @ts-expect-error for untyped utils |
examples/bpk-component-label/stories.tsx |
Updated Storybook story syntax for VisualTestWithZoom |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| children: ReactNode; | ||
| className?: string; | ||
| disabled?: boolean; | ||
| valid?: boolean | 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.
The valid is supposed to be a boolean and only has 2 values, but it requires changing the code below. To keep the TS migration pure and safe, keep it as it was for now.
| disabled = false, | ||
| required = false, | ||
| valid = null, | ||
| valid, |
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.
it seems valid removed default null value?
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.
Hi Copilot add the null as a default value to valid.
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 the default value null to the valid parameter in commit 6a2a540. This matches the original JavaScript implementation.
| children: ReactNode; | ||
| className?: string; | ||
| disabled?: boolean; | ||
| valid?: boolean | 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.
There is nowhere use null value for valid, and in L45 only handle false value without null and undefined, should it clearly distinguish between null and undefined?
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.
Hi Ezreal (@Ezreal-Yang), thanks for asking! I have considered this question. Currently, the valid property has three values: true, false, and null, which seems unnecessary. In my opinion, it only needs two values to control its style effectively. It should display the invalid style only when valid is set to false.
I'm unsure how this will be used downstream, especially in cases where valid might be set to null. If I modify it directly, it could lead to a type error in its usage. As I mentioned in my comment, I prefer to keep it as it is in this PR.
|
Visit https://backpack.github.io/storybook-prs/4110 to see this build running in a browser. |
Restore the original default value for the valid parameter to match the original JavaScript implementation. Co-authored-by: Faye-Xiao <20058385+Faye-Xiao@users.noreply.github.com>
|
Visit https://backpack.github.io/storybook-prs/4110 to see this build running in a browser. |
|
Hi Copilot fix the type error: packages/bpk-component-theme-toggle/src/BpkThemeToggle.tsx(22,1): error TS2578: Unused 'ts-expect-error' directive. |
Since BpkLabel is now migrated to TypeScript, the @ts-expect-error directive is no longer needed in bpk-component-theme-toggle. Co-authored-by: Faye-Xiao <20058385+Faye-Xiao@users.noreply.github.com>
Fixed in commit 60d75a9. Removed the unused |
|
Visit https://backpack.github.io/storybook-prs/4110 to see this build running in a browser. |
|
Approved the precy failure https://percy.io/09e69e44/web/backpack/builds/45962076/changed/2429138062?browser=chrome&browser_ids=69&group_snapshots_by=similar_diff&subcategories=unreviewed%2Cchanges_requested&utm_source=github_status_public&viewLayout=side-by-side&viewMode=new&width=1280&widths=375%2C1280 as some test cases are flaky tests due to a generation problem, not real test error. |
Migrated BpkLabel from Flow to TypeScript with improved type safety and test coverage.
Changes
Component (
BpkLabel.js→BpkLabel.tsx)ComponentPropsWithoutRef<'label'>for proper HTML attribute typingvalidprop type toboolean | null | undefinedto match original Flow type (?boolean)nullfor thevalidparameter to preserve original behaviorTests (
BpkLabel-test.js→BpkLabel-test.tsx)@testing-library/jest-domSupporting files
.tsx@ts-expect-errordirective for untypedbpk-storybook-utilsimportDependent component updates
@ts-expect-errordirectives frombpk-component-textarea,bpk-component-fieldset,bpk-component-nudger, andbpk-component-theme-togglethat were importing BpkLabel (now typed)Type Safety Improvements
The intersection with
ComponentPropsWithoutRef<'label'>provides proper typing for all standard HTML label attributes (e.g.,htmlFor,onClick) while maintaining custom props. Thevalidprop acceptsboolean | null | undefinedto maintain compatibility with the original Flow type, with a default value ofnullmatching the original implementation.Remember to include the following changes:
[Clover-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.mdOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.