backpack
backpack copied to clipboard
[BD-6195] contribute component - bpk page indicator
Figma link: https://www.figma.com/file/yN0hFyZlKL0Jwbpi0rEKYT/Backpack-Beta?node-id=1763%3A18266
Remember to include the following changes:
- [x]
UNRELEASED.md
(See How to write a good changelog entry) - [x]
README.md
- [x] Tests
- [x] Docs (followed TK has been created)
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
Visit https://backpack.github.io/storybook-prs/2604 to see this build running in a browser.
Visit https://backpack.github.io/storybook-prs/2604 to see this build running in a browser.
Visit https://backpack.github.io/storybook-prs/2604 to see this build running in a browser.
Would you be able to check the overImage example? For some reason I am not seeing the indicators when I open this
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.
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?
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): - 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.
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):![]()
- 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!
Love the PR, thank you @giriawu ❤️
Visual regression tests passed 😎. Bear in mind that they only run in Chromium on static components – they aren't perfect.
Visit https://backpack.github.io/storybook-prs/2604 to see this build running in a browser.
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.
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.
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
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🤔
Visual regression tests passed 😎. Bear in mind that they only run in Chromium on static components – they aren't perfect.
Visit https://backpack.github.io/storybook-prs/2604 to see this build running in a browser.
I've rerun
npm run storybook:dist
andnpm run jest:visual-tests:update
locally, but it cannot pass the visual test. I've passed once, but the CI snapshot aboutbpk-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 🙂