-
Notifications
You must be signed in to change notification settings - Fork 205
[PANGOO-2945][BpkPageIndicator]Bpk component page indicator ts support #3783
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
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Browser supportIf this is a visual change, make sure you've tested it in multiple browsers. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
…s://github.com/Skyscanner/backpack into bpk-component-page-indicator-TS-support-Part-1
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 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 adds TypeScript support for the BpkPageIndicator component and updates related files to remove Flow types and PropTypes while improving type safety.
- Migrated components from Flow to TypeScript by replacing PropTypes and defaultProps with proper interfaces and default values.
- Updated related tests, examples, and the carousel integration to align with the new type definitions.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/bpk-component-page-indicator/src/NavButton.tsx | Updated type definitions and removed PropTypes to use TypeScript's type system. |
packages/bpk-component-page-indicator/src/BpkPageIndicator.tsx | Replaced PropTypes and Flow types with TypeScript interfaces and default values. |
packages/bpk-component-page-indicator/src/BpkPageIndicator-test.tsx | Improved test props typing for better type safety. |
packages/bpk-component-page-indicator/index.ts | Adjusted type imports and exports for consistency. |
packages/bpk-component-carousel/src/BpkCarousel.tsx | Removed the ts-expect-error suppression; ensure compatibility with updated types. |
examples/bpk-component-page-indicator/stories.tsx | Updated Storybook stories using the CSF object notation. |
examples/bpk-component-page-indicator/examples.tsx | Added explicit type annotations to example container component props. |
Comments suppressed due to low confidence (1)
packages/bpk-component-carousel/src/BpkCarousel.tsx:20
- The removal of the ts-expect-error suppression comment implies that the type definitions for BpkPageIndicator should now be correct. Please verify that no TypeScript errors are introduced due to this change.
// @ts-expect-error Untyped import. See `decisions/imports-ts-suppressions.md`.
Visit https://backpack.github.io/storybook-prs/3783 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.
we also need to remove the /* @flow strict */
in packages/bpk-component-page-indicator/src/accessibility-test.tsx
describe('BpkPageIndicator', () => { | ||
let 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.
I think it would be better if we reuse the type defined in packages/bpk-component-carousel/src/BpkCarousel.tsx
totalIndicators - DISPLAYED_TOTAL, | ||
) | ||
: 0, | ||
} as React.CSSProperties |
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.
As I remember in banana, we are not recommending to use type assertion, therefore, i would prefer
import type { CSSProperties } from 'react';
...
type CustomCSSProperties = CSSProperties & {
'--scroll-index'?: number;
}
const customStyle: CustomCSSProperties = {
'--scroll-index':
totalIndicators > DISPLAYED_TOTAL
? Math.min(
currentIndex - START_SCROLL_INDEX,
totalIndicators - DISPLAYED_TOTAL,
)
: 0,
}
<div
className={getClassName('bpk-page-indicator__indicators-container')}
style={
currentIndex > START_SCROLL_INDEX
? customStyle
: undefined
}
>
...
I noticed the Percy failed here, I don't think the ts migration would cause the visual changes, could you help to check with it? |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
1 similar comment
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3783 to see this build running in a browser. |
…s://github.com/Skyscanner/backpack into bpk-component-page-indicator-TS-support-Part-1
Visit https://backpack.github.io/storybook-prs/3783 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.
LGTM
Remember to include the following changes:
[Clover-123][BpkButton] Updating the colour
README.md
(If you have created a new component)README.md