Conversation
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
|
|
||
| display: flex; | ||
| overflow-x: hidden; | ||
| overflow-x: scroll; |
| // Negative margin to ensure box-shadow of content is not cut off | ||
| margin-block: -(tokens.bpk-spacing-lg()); | ||
| margin-inline: -(tokens.bpk-spacing-md()); | ||
| -webkit-overflow-scrolling: touch; |
There was a problem hiding this comment.
enables native scroll on safari
|
|
||
| @include breakpoints.bpk-breakpoint-above-mobile { | ||
| // On tablet and desktop, only snap to page-start cards for page-based scrolling | ||
| scroll-snap-align: none; |
There was a problem hiding this comment.
this means not specifying where on the card to snap to so no snap scroll
because logic to snap every so cards will be determined elsewhere
| &__card--page-start { | ||
| @include breakpoints.bpk-breakpoint-above-mobile { | ||
| scroll-snap-align: start; | ||
| scroll-snap-stop: always; |
There was a problem hiding this comment.
'always' makes it so it stops at the next card as opposed to 'normal', the same amount of force on the trackpad will scroll through more options
|
|
||
| const stateScrollingLockRef = useRef(false); | ||
| const openSetStateLockTimeoutRef = useRef<NodeJS.Timeout | null>(null); | ||
| const [lockVersion, setLockVersion] = useState(0); |
There was a problem hiding this comment.
state to share between page-based scrolling and the button navigation
…ng and button nav is a bit broken
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
| // Mobile: scroll to individual card (use currentIndex directly) | ||
| useScrollToCard( | ||
| currentIndex * initiallyShownCards, | ||
| isMobile ? currentIndex : currentIndex * initiallyShownCards, |
There was a problem hiding this comment.
separating mobile experience that snaps to every card
| root, | ||
| cardRefs, | ||
| stateScrollingLockRef, | ||
| openSetStateLockTimeoutRef, |
There was a problem hiding this comment.
not sure what this is yet
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
| useEffect(() => { | ||
| const firstVisible = visibilityList.indexOf(1); | ||
| if (firstVisible >= 0) { | ||
| if (firstVisible >= 0 && stateScrollingLockRef.current) { |
There was a problem hiding this comment.
Only set the index from within our component if we're locked, meaning there has been a user scrolling interaction
|
|
||
| const commonProps = { | ||
| className: getClassName(`bpk-card-list-row-rail__${layout}__card`), | ||
| className: `${getClassName(`bpk-card-list-row-rail__${layout}__card`)}${isPageStart ? ` ${getClassName('bpk-card-list-row-rail__card--page-start')}` : ''}`, |
There was a problem hiding this comment.
Add the page start classes to the correct cards
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
| usePageScrollSync({ | ||
| currentIndex, | ||
| setCurrentIndex, | ||
| initiallyShownCards, | ||
| cardRefs, | ||
| stateScrollingLockRef, | ||
| ); | ||
| visibilityList, | ||
| container: root, | ||
| enabled: !isMobile, | ||
| }); |
There was a problem hiding this comment.
refactor various scroll related logic into a consolidated hook to make it easier to reason about and control the various scroll mechanisms
| useEffect(() => { | ||
| const container = root; | ||
| if (isMobile || !container) return undefined; | ||
|
|
||
| const lockScrollDuringInteraction = () => { | ||
| lockScroll(stateScrollingLockRef, openSetStateLockTimeoutRef); | ||
| }; | ||
|
|
||
| container.addEventListener('wheel', lockScrollDuringInteraction); | ||
| container.addEventListener('touchmove', lockScrollDuringInteraction); | ||
|
|
||
| return () => { | ||
| container.removeEventListener('touchmove', lockScrollDuringInteraction); | ||
| container.removeEventListener('wheel', lockScrollDuringInteraction); | ||
| if (openSetStateLockTimeoutRef.current) { | ||
| clearTimeout(openSetStateLockTimeoutRef.current); | ||
| } | ||
| }; | ||
| }, [root]); |
There was a problem hiding this comment.
This responsibility moved to new usePageScrollSync hook
| useEffect(() => { | ||
| const firstVisible = visibilityList.indexOf(1); | ||
| if (firstVisible >= 0) { | ||
| const newIndex = Math.floor(firstVisible / initiallyShownCards); | ||
| if (newIndex !== currentIndex) { | ||
| setCurrentIndex(newIndex); | ||
| } | ||
| } | ||
| }, [initiallyShownCards]); |
There was a problem hiding this comment.
This responsibility moved to new usePageScrollSync hook
| }); | ||
| }; | ||
|
|
||
| export const useScrollToCard = ( |
There was a problem hiding this comment.
This responsibility moved to new usePageScrollSync hook
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
| className: getClassName(`bpk-card-list-row-rail__${layout}__card`), | ||
| className: getClassName( | ||
| `bpk-card-list-row-rail__${layout}__card`, | ||
| isPageStart && 'bpk-card-list-row-rail__card--page-start', |
There was a problem hiding this comment.
Page start to allow the scroll snapping styles 🚀
| container.getBoundingClientRect().bottom > 0 && | ||
| container.getBoundingClientRect().bottom <= window.innerHeight; | ||
|
|
||
| if (!isVisible) { |
There was a problem hiding this comment.
early return if not visible to not scroll
| const targetCard = cardRefs.current?.[currentIndex * initiallyShownCards]; | ||
| if (!targetCard) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
early return if the target doesn't exist, sensible
Jimmy cook (jimmycook)
left a comment
There was a problem hiding this comment.
looks good from Irn Bru side
|
|
||
| export const RELEASE_LOCK_DELAY = 20; | ||
| export const RENDER_BUFFER_SIZE = 3; No newline at end of file | ||
| export const RELEASE_LOCK_DELAY = 200; |
There was a problem hiding this comment.
Is 200ms derived from empirical testing across trackpad/wheel/iPad touch? Any risk of sluggish page indicator updates?
There was a problem hiding this comment.
Hi - yes 200 was a sweetspot value that I arrived at after testing on multiple devices and browsers. At this value there is no flickering of the page index indicators (even on lower powered devices), and the page indicator updates feel natural and not at all sluggish
| cardDimensionStyle.height = `${firstCardHeightRef.current}px`; | ||
| } | ||
|
|
||
| const isPageStart = index % initiallyShownCards === 0; |
There was a problem hiding this comment.
Minor thought: since page-start is calculated via index % initiallyShownCards, do we handle responsive changes and the “incomplete last page” case as expected? Might be worth a quick test just to make sure snapping always lands on a valid page start.
There was a problem hiding this comment.
Good point! I've had a look at a storybook example with 13 cards. It appears to work as expected and handles responsive changes well too. When we move from index 9 to 12, card visibility goes from [9, 10, 11], to [10, 11, 12] which is intuitive behaviour. With page index still at 12 and responsively moving to mobile, visibility is snapped to [12] as expected
bpk-cardlist-responsive.webm
|
Visit https://backpack.github.io/storybook-prs/4138 to see this build running in a browser. |
Summary
This PR adds native physical scrolling behavior to the BpkCardListCarousel component on desktop and tablet devices, replacing the previous button-only navigation with a more natural scrolling experience while maintaining the page-based navigation model.
Changes
Key improvements:
gestures on desktop/tablet, with scrolling snapping to pages rather than individual cards
(pagination buttons) and user-initiated scrolling (wheel/touch), preventing conflicts between the two
usePageScrollSync hook that uses a silence-based lock release to distinguish between user and
programmatic scrolling
Technical changes:
overflow-x: hiddentooverflow-x: scrollon desktop/tabletcard of each page snaps on desktop/tablet)
scroll-snap-stop: alwaysfrom all cards and applied it only topage-start cards on desktop/tabletTesting
This has been manually tested on:
Desktop:
iPad:
Android phone:
iPhone:
Remember to include the following changes:
[Clover-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.md