[BD-8468][BpkCarousel] new component for mobile #3059
Conversation
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
Co-authored-by: Ollie Curtis <[email protected]>
…e.scss Co-authored-by: Ollie Curtis <[email protected]>
Co-authored-by: Ollie Curtis <[email protected]>
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
Co-authored-by: Ollie Curtis <[email protected]>
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
…into bpk-carousel
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
KathyWang0208
changed the title
KathyWang0208
changed the title
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
| images: ReactNode[] | ||
| initialImageIndex?: number; | ||
| onImageChanged?: OnImageChangedHandler | ||
| bottom?: number; |
There was a problem hiding this comment.
What is the purpose of this property?
There was a problem hiding this comment.
Looking at the figma definition it doesn't seem to be included there.
If we need this then it would be good for us to add a comment above in the following format, so that our autogenerated docs can pick this up and make it clear to consumers what it does
/**
* Put your description here
*/
bottom?: number;| @@ -36,19 +36,19 @@ export const VARIANT = { | |||
| }; | |||
|
|
|||
| export type Props = { | |||
| className: ?string, | |||
There was a problem hiding this comment.
This is going to cause a major release by removing this property for consumers so we need to be careful in its removal, as there is now going to be consumer impact
There was a problem hiding this comment.
I was hesitant to get rid of it because a recent rule is not to recommend className.
Based on your comment, I am confident to revert the change, thank you.
| indicatorLabel: ?string, | ||
| prevNavLabel: ?string, | ||
| nextNavLabel: ?string, |
There was a problem hiding this comment.
What would be the reason for moving these to optional? Seems a significant change/risk to non carousel users
There was a problem hiding this comment.
In carousel, we don't need to pass the onClick function to BpkPageIndicator, and these label are used to render the button(only onClick is available, the button is rendered), so here I changed them as optional.
But I also worried about what you said, so I reverted the changes, and set the default en label in carousel(they won't be used in BpkPageIndicator, so they don't introduce the localization issues I think).
| variant: ?$Keys<typeof VARIANT>, | ||
| bottom: ?number, |
There was a problem hiding this comment.
Similar to previous
What is the purpose of this property?
If we need this then it would be good for us to add a comment above in the following format, so that our autogenerated docs can pick this up and make it clear to consumers what it does
/** * Put your description here */ bottom?: number;
| <div className={className}> | ||
| }: Props) => { | ||
| const isOverImage = variant === VARIANT.overImage; | ||
| const isInteractive = !!onClick; |
There was a problem hiding this comment.
Could you clarify or add a comment as the purpose of this?
There was a problem hiding this comment.
For isOverImage, why I did this change before I was thinking this component should include the corresponding css when customer pass VARIANT.overImage and in order to be compatible with the custom bottom spacing, customer need pass one more prop bottom, it seems too many changes for this existing component, so I moved this css for overImage to BpkCarousel.
For isInteractive, I will add some comments.
|
Doing a quick test for interaction andTalkBack test on Android, it looks like when the component first loads image one is last and the last indicator dot is highlighted, so when you scroll though its like it starts on the last element and then when you get the last element the first dot is highlighted. Wondering if this is related to this question about logic I had in the past is in the reverse order? https://github.com/Skyscanner/backpack/pull/3059/files#r1382019852 |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
Yes, in order for the scroll snap to create an infinite loop, here render extra two images. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3059 to see this build running in a browser. |
Added carousel component for mobile screens which displays images which can be swiped to move onto the next image.
Remember to include the following changes:
README.md(If you have created a new component)README.md