Skip to content

[CLOV-119] Migrate bpk-component-progress JS to TypeScript#4486

Merged
Vincent Liu (xiaogliu) merged 2 commits into
mainfrom
xiaogliu/progress-ts-migration
May 15, 2026
Merged

[CLOV-119] Migrate bpk-component-progress JS to TypeScript#4486
Vincent Liu (xiaogliu) merged 2 commits into
mainfrom
xiaogliu/progress-ts-migration

Conversation

@xiaogliu
Copy link
Copy Markdown
Contributor

@xiaogliu Vincent Liu (xiaogliu) commented May 14, 2026

Summary

  • Convert the bpk-component-progress package from JavaScript/Flow to TypeScript
  • Rename source/test files (.js.ts/.tsx) and the snapshot file so Jest still finds it
  • Replace propTypes + Flow Props with a typed Props export, re-exported as BpkProgressProps
  • Type Props as Omit<HTMLAttributes<HTMLDivElement>, 'className'> & { ... } so native div attrs flow through ...rest while keeping className?: string | null
  • Replace propTypes + defaultProps with static defaultProps; destructure onComplete/onCompleteTransitionEnd out of rest instead of the legacy delete rest.x workaround
  • tabIndex="0"tabIndex={0} so the prop type-checks; rendered DOM is unchanged (tabindex="0")
  • Type both class containers in the stories file with explicit Props/State generics
  • Narrow getValueText return type from unknown to string (addresses review feedback) — non-string returns would have been DOM-stringified to "[object Object]" and announced by screen readers
  • No business-logic, JSX, or rendering changes — existing snapshots remain valid

Notable type narrowings (theoretical breaking, runtime unchanged)

  • tabIndex={null} no longer type-checks — the new Props inherits tabIndex?: number from HTMLAttributes<HTMLDivElement> (was Flow ?number). Pass undefined or omit the prop instead. Runtime DOM behavior is unchanged.
  • getValueText must now return string (was Flow mixed). Existing valid usages already do.

Test plan

  • npm run jest -- packages/bpk-component-progress (3 suites, 10 tests, 5 snapshots all pass)
  • npx tsc --noEmit — no errors in bpk-component-progress
  • npx eslint packages/bpk-component-progress — no errors, no warnings

🤖 Generated with Claude Code

Convert the bpk-component-progress package from JavaScript/Flow to
TypeScript. Replace prop-types and Flow types with a typed `Props`
export and rename all source/test files (.js -> .ts/.tsx). No
business-logic, JSX, or rendering changes - existing snapshots remain
valid.

- Rename index.js -> index.ts and re-export `BpkProgressProps` type
- Rename BpkProgress.js -> BpkProgress.tsx; type `Props` extends
  `Omit<HTMLAttributes<HTMLDivElement>, 'className'>` so native div
  props flow through `...rest` while keeping `className?: string | null`
- Replace `propTypes` + `defaultProps` with TS `static defaultProps`
- Destructure `onComplete`/`onCompleteTransitionEnd` out of `rest`
  (drops the legacy `delete rest.x` workaround)
- `tabIndex="0"` -> `tabIndex={0}` so the prop type-checks; rendered
  DOM (`tabindex="0"`) is unchanged and snapshots still match
- Type both class containers in the stories file with explicit
  `Props`/`State` generics
- Rename themeAttributes and test files; remove `@flow strict` headers
- Rename snapshot file to BpkProgress-test.tsx.snap so Jest finds it

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 14, 2026 02:19
@xiaogliu Vincent Liu (xiaogliu) added the minor Non breaking change label May 14, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a 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 bpk-component-progress from Flow/JavaScript to TypeScript while preserving the component’s runtime behavior and snapshots.

Changes:

  • Converts progress component, tests, stories, theme attributes, and package entry point to TypeScript.
  • Replaces Flow/PropTypes definitions with exported TypeScript props.
  • Renames and preserves Jest snapshots for the TypeScript test file.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/bpk-component-progress/src/BpkProgress.tsx Adds TypeScript props, defaults, and typed helper signatures for the progress component.
packages/bpk-component-progress/src/BpkProgress.stories.tsx Adds explicit props/state types to story containers.
packages/bpk-component-progress/src/BpkProgress-test.tsx Removes Flow annotation from migrated test file.
packages/bpk-component-progress/src/accessibility-test.tsx Removes Flow annotation from migrated accessibility test.
packages/bpk-component-progress/src/themeAttributes.ts Removes Flow annotation from theme attributes module.
packages/bpk-component-progress/src/themeAttributes-test.ts Removes Flow annotation from migrated theme attributes test.
packages/bpk-component-progress/src/__snapshots__/BpkProgress-test.tsx.snap Adds renamed snapshot file for the TypeScript test.
packages/bpk-component-progress/index.ts Re-exports the component and its TypeScript props type.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@skyscanner-backpack-bot
Copy link
Copy Markdown

Visit https://backpack.github.io/storybook-prs/4486 to see this build running in a browser.

@skyscanner-backpack-bot
Copy link
Copy Markdown

skyscanner-backpack-bot Bot commented May 14, 2026

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against c54f20e

@xiaogliu Vincent Liu (xiaogliu) changed the title [BpkProgress] Migrate JS to TypeScript [CLOV-119] Migrate bpk-component-progress JS to TypeScript May 14, 2026
Copy link
Copy Markdown
Contributor

@kerrie-wu kerrie-wu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Generated with Claude Code

role="progressbar"
aria-valuetext={getValueText ? getValueText(value, min, max) : null}
aria-valuetext={
getValueText ? (getValueText(value, min, max) as string) : undefined
Copy link
Copy Markdown
Contributor

@kerrie-wu kerrie-wu May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as string masks an a11y contract gap (recurring pattern)

getValueText is typed (...) => unknown at L58, so as string here only silences TS. At runtime a consumer returning a non-string (number, object, ReactNode) is coerced to "[object Object]" and announced to assistive tech — visually invisible, but an a11y regression.

Same anti-pattern flagged on #4475 (BpkRadio): "This cast only satisfies TypeScript; it still sends non-string React nodes to aria-label at runtime, producing names like [object Object]."

Fix — narrow the source type instead of casting at the use site:

// L58
getValueText?: ((value: number, min: number, max: number) => string) | null;

// L127
aria-valuetext={getValueText ? getValueText(value, min, max) : undefined}

The README example already returns a string, so no break for valid usage; bad callers like () => 42 now fail at compile time instead of producing [object Object] at runtime.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c54f20e — narrowed getValueText return type to string and dropped the as string cast. Non-string returns now fail at compile time.

stepped: boolean,
small: boolean,
className: ?string,
tabIndex: ?number,
Copy link
Copy Markdown
Contributor

@kerrie-wu kerrie-wu May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: tabIndex no longer accepts null.

Old Flow type was tabIndex: ?number (accepts null); the new TS Props inherits tabIndex?: number from HTMLAttributes, so tabIndex={null} is now a TS error. Unlikely to bite in practice — tabIndex={null} is uncommon — but worth a one-liner in the PR description for completeness

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — added a note to the PR description under "Notable type narrowings". Decided not to widen Props.tabIndex back to number | null so the type stays consistent with the inherited HTMLAttributes<HTMLDivElement> shape (matches BpkRadio etc.). Runtime behavior is unchanged.

Address review feedback: the previous `as string` cast satisfied
TypeScript but still let consumers return non-string values, which
the DOM stringifies to "[object Object]" — a poor accessible name
announced by screen readers.

Tighten `getValueText`'s return type from `unknown` to `string` so
non-conforming callers fail at compile time instead of producing
runtime a11y regressions. Drop the now-unnecessary cast at the
`aria-valuetext` use site.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@skyscanner-backpack-bot
Copy link
Copy Markdown

Visit https://backpack.github.io/storybook-prs/4486 to see this build running in a browser.

Copy link
Copy Markdown
Contributor

@kerrie-wu kerrie-wu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xiaogliu Vincent Liu (xiaogliu) merged commit 05598cb into main May 15, 2026
14 checks passed
@xiaogliu Vincent Liu (xiaogliu) deleted the xiaogliu/progress-ts-migration branch May 15, 2026 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor Non breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants