backpack icon indicating copy to clipboard operation
backpack copied to clipboard

[BD-6195] contribute component - bpk page indicator

Open giriawu opened this issue 1 year ago • 3 comments

Figma link: https://www.figma.com/file/yN0hFyZlKL0Jwbpi0rEKYT/Backpack-Beta?node-id=1763%3A18266

image Remember to include the following changes:

giriawu avatar Sep 30 '22 10:09 giriawu

Browser support

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

Generated by :no_entry_sign: dangerJS against cc6c7240c7a3acef315fa8c8806b4b7d536f6c23

github-actions[bot] avatar Oct 10 '22 06:10 github-actions[bot]

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

github-actions[bot] avatar Oct 13 '22 09:10 github-actions[bot]

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

github-actions[bot] avatar Oct 18 '22 02:10 github-actions[bot]

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

github-actions[bot] avatar Oct 20 '22 11:10 github-actions[bot]

Would you be able to check the overImage example? For some reason I am not seeing the indicators when I open this

olliecurtis avatar Oct 20 '22 22:10 olliecurtis

Would you be able to check the overImage example? For some reason I am not seeing the indicators when I open this

I've now resolve the conflicts and republish it. On my side it shows up fine, maybe you need to scroll down a bit to see it (as it is at the bottom of the picture). Or you can see it in the visual test.

giriawu avatar Oct 21 '22 03:10 giriawu

Would you be able to check the overImage example? For some reason I am not seeing the indicators when I open this

This does seem a bit strange, in visual test example it is displayed normally. I have tried to republish.

Could it be missed because the indicators are at the bottom of the image instead of the middle? image

jasonhuang-sky avatar Oct 21 '22 03:10 jasonhuang-sky

We will want to re-use this component in the image gallery, but there are some differences:

  • it uses Non-finite indicators (transform: scale applied): image
  • the indicators are not clickable, as they are small targets and only used on mobile. My main concern with making them tabbable with keyboard is that keyboard users would need to Tab through potentially over 40+ indicators. In these cases we could implement it like Radio Group (example or Tabs: there's only 1 tabbable element – active one – so that users Tab In to the indicators rail and Tab Out of it with single Tab. The navigation between indicators is than handled by the arrow keys.

LexSwed avatar Oct 21 '22 07:10 LexSwed

We will want to re-use this component in the image gallery, but there are some differences:

  • it uses Non-finite indicators (transform: scale applied): image
  • the indicators are not clickable, as they are small targets and only used on mobile. My main concern with making them tabbable with keyboard is that keyboard users would need to Tab through potentially over 40+ indicators. In these cases we could implement it like Radio Group (example or Tabs: there's only 1 tabbable element – active one – so that users Tab In to the indicators rail and Tab Out of it with single Tab. The navigation between indicators is than handled by the arrow keys.

Hi, first of all, thanks for your review! I think your advice is good. But my consideration is that the indicator should have the role of navigation as a whole, in addition to the logic of its own Radio state. It should by default have semantics such as page-turning/page-indicatoring, because in practice it is often used in combination with multiple tab pages, or images. Regarding component interaction, I think we can design more function and improvement in the future with the backpack team and designers and engineers in the following step. Thanks again for your comment!

giriawu avatar Oct 21 '22 09:10 giriawu

Love the PR, thank you @giriawu ❤️

LexSwed avatar Oct 22 '22 10:10 LexSwed

Visual regression tests passed 😎. Bear in mind that they only run in Chromium on static components – they aren't perfect.

github-actions[bot] avatar Oct 25 '22 06:10 github-actions[bot]

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

github-actions[bot] avatar Oct 25 '22 06:10 github-actions[bot]

I'm not sure why this happens. But after I merge master, it tells me that I must update the snapshot in the bpk-component-content-cards, otherwise, the visual test will fail. The error log is here: https://github.com/Skyscanner/backpack/actions/runs/3317969295/jobs/5481397876 So I update it in this PR.

giriawu avatar Oct 25 '22 08:10 giriawu

Visual regression tests failed 😢. You can download the failure diffs from the 'Artifacts' section of the failed CI run. To update the tests, run npm run jest:visual-tests:update locally.

github-actions[bot] avatar Oct 25 '22 10:10 github-actions[bot]

I'm not sure why this happens. But after I merge master, it tells me that I must update the snapshot in the bpk-component-content-cards, otherwise, the visual test will fail. The error log is here: https://github.com/Skyscanner/backpack/actions/runs/3317969295/jobs/5481397876 So I update it in this PR.

Hi @giriawu sorry about this, I'm having a look, but I can't access the uploaded artifacts not sure why 😢 does that work for you? there shouldn't be any differences between the screenshots though

anambl avatar Oct 25 '22 10:10 anambl

I've rerun npm run storybook:dist and npm run jest:visual-tests:update locally, but it cannot pass the visual test. I've passed once, but the CI snapshot about bpk-component-content-cards changed again🤔

giriawu avatar Oct 25 '22 10:10 giriawu

Visual regression tests passed 😎. Bear in mind that they only run in Chromium on static components – they aren't perfect.

github-actions[bot] avatar Oct 25 '22 10:10 github-actions[bot]

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

github-actions[bot] avatar Oct 25 '22 10:10 github-actions[bot]

I've rerun npm run storybook:dist and npm run jest:visual-tests:update locally, but it cannot pass the visual test. I've passed once, but the CI snapshot about bpk-component-content-cards changed again🤔

I see it passed on CI, does it fail locally? 🤔 no worries about it though, nothing for you to fix 🙂

anambl avatar Oct 25 '22 11:10 anambl