Skip to content
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.

Sign up for GitHub

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

Jump to bottom

backpack carousel #3059

Closed
wants to merge 21 commits into from
Closed

backpack carousel #3059

wants to merge 21 commits into from

Conversation

R1CC1M
Copy link
Contributor

@R1CC1M R1CC1M commented Nov 3, 2023 •
edited

Added carousel component for mobile screens which displays images which can be swiped to move onto the next image.

Screenshot 2023-11-03 at 13 15 26

Remember to include the following changes:

  • README.md (If you have created a new component)
  • Component README.md
  • Tests
  • Storybook examples created/updated
  • For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

@R1CC1M R1CC1M added the minor Non breaking change label Nov 3, 2023
@olliecurtis olliecurtis self-requested a review November 3, 2023 09:47
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023 •
edited

Browser support

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

Generated by 🚫 dangerJS against 6999c0d

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

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

olliecurtis
olliecurtis requested changes Nov 3, 2023
View reviewed changes
packages/bpk-component-carousel/README.md Outdated Show resolved Hide resolved
packages/bpk-component-carousel/src/index.ts Outdated Show resolved Hide resolved
examples/bpk-component-carousel/example.js Show resolved Hide resolved
examples/bpk-component-carousel/stories.js Show resolved Hide resolved
examples/bpk-component-carousel/example.js Outdated Show resolved Hide resolved
packages/bpk-component-carousel/src/accessibility-test.tsx Outdated Show resolved Hide resolved
packages/bpk-component-carousel/src/types.ts Outdated Show resolved Hide resolved
packages/bpk-component-carousel/src/utils.tsx Outdated Show resolved Hide resolved
packages/bpk-component-carousel/src/BpkCarouselContainer.tsx Show resolved Hide resolved
packages/bpk-component-carousel/src/BpkCarouselContainer.module.scss Show resolved Hide resolved
R1CC1M and others added 3 commits November 3, 2023 12:20
@R1CC1M @olliecurtis
Update packages/bpk-component-carousel/README.md
dc3964b 
Co-authored-by: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com>
@R1CC1M @olliecurtis
Update packages/bpk-component-carousel/src/BpkCarouselContainer.modul…
419970d 
…e.scss

Co-authored-by: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com>
@R1CC1M @olliecurtis
Update packages/bpk-component-carousel/src/BpkCarouselImage.module.scss
54a4e82 
Co-authored-by: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com>
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

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

olliecurtis
olliecurtis requested changes Nov 3, 2023
View reviewed changes
Copy link
Member

@olliecurtis olliecurtis left a comment

Choose a reason for hiding this comment

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

Couple of minor changes and otherwise this looks good to me

examples/bpk-component-carousel/stories.js Show resolved Hide resolved
packages/bpk-component-carousel/README.md Outdated Show resolved Hide resolved
R1CC1M and others added 2 commits November 3, 2023 14:36
@R1CC1M @olliecurtis
Update packages/bpk-component-carousel/README.md
c59f9f2 
Co-authored-by: Ollie Curtis <8831547+olliecurtis@users.noreply.github.com>
@R1CC1M
visual test added
324b380
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2023

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

olliecurtis
olliecurtis previously approved these changes Nov 3, 2023
View reviewed changes
olliecurtis
olliecurtis reviewed Nov 3, 2023
View reviewed changes
Copy link
Member

@olliecurtis olliecurtis left a comment

Choose a reason for hiding this comment

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

Just going to run an A11y test to make sure that VO and TB answer as expected

@olliecurtis olliecurtis self-requested a review November 3, 2023 17:31
olliecurtis
olliecurtis reviewed Nov 3, 2023
View reviewed changes
packages/bpk-component-carousel/src/BpkCarouselContainer.tsx
Comment on lines +93 to +99
<BpkCarouselImage
image={images[0]}
index={0}
ref={(el) => {
observeCycleScroll(el);
observeImageChange(el);
}}
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be before the logic to map each of the images?
As it seems to always have the first selected image as the last one when it renders which is this expected behaviour?

Copy link

Choose a reason for hiding this comment

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

Ricci is away on AL now, but it seems to be in order for the scroll snap to create an infinite loop.

@olliecurtis olliecurtis self-requested a review November 4, 2023 22:53
@olliecurtis olliecurtis dismissed their stale review November 4, 2023 22:54

Reviewing Accessibility

@github-actions GitHub Actions
Copy link

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

@github-actions GitHub Actions
Copy link

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

@olliecurtis
Copy link
Member

Completed A11y testing on this component and with a screen reader exhibits behaviour that causes focus to go through more items than available and read outs to not be as correct for users with A11y requirements

@olliecurtis olliecurtis marked this pull request as draft January 19, 2024 10:33
@github-actions GitHub Actions
Copy link

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

@anambl
Copy link
Contributor

anambl commented May 6, 2024 •
edited

@R1CC1M @JafferN is there any work being done currently on this or should I close the PR? 🙂

@anambl anambl closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JafferN JafferN JafferN left review comments

@olliecurtis olliecurtis Awaiting requested review from olliecurtis

Assignees
No one assigned
Labels
minor Non breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@R1CC1M @olliecurtis @anambl @JafferN