[Clover-402][BpkCardList] Update BpkCardList scroll behaviour #3878
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,7 +129,6 @@ const commonProps = { | |
| ), | ||
| buttonContent: 'See more', | ||
| buttonHref: 'https://www.skyscanner.net/', | ||
|
There was a problem hiding this comment. You removed this value, but I saw there is an example of giving There was a problem hiding this comment. I removed it here is because I change mobile default But in this case, we want to test different initial cards so still keep it. |
||
| accessibilityLabels: { | ||
| indicatorLabel: 'Go to slide', | ||
| prevNavLabel: 'Previous slide', | ||
|
|
@@ -152,7 +151,6 @@ const snippetProps = { | |
| }; | ||
|
|
||
| const DestinationCard = (i: number) => ( | ||
|
|
||
| <BpkCard | ||
| key={`card-${i}`} | ||
| className={STYLES['bpk-card']} | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,27 +17,33 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @use '../../../unstable__bpk-mixins/tokens'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| @use '../../../unstable__bpk-mixins/breakpoints'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| @use '../../../unstable__bpk-mixins/utils'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| .bpk-card-list-row-rail { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| &__row, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| &__rail { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| display: flex; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| padding-top: tokens.bpk-spacing-sm(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| padding-bottom: tokens.bpk-spacing-sm(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| overflow-x: hidden; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. In desktop, scroll card by wheel/touchpad will change currentIndex, And pagination will also change the currentIndex. It is rather difficult to keep the two in perfect synchronization, and the functionality is redundant with that of pagination so disable this feature. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| box-sizing: border-box; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| gap: tokens.bpk-spacing-sm(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| scroll-snap-stop: always; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| scroll-snap-type: x mandatory; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| scrollbar-width: none; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @include breakpoints.bpk-breakpoint-mobile { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| overflow-x: scroll; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. [nitpick] There are two separate
Suggested change
Copilot uses AI. Check for mistakes. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| &::-webkit-scrollbar { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| display: none; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| &__card { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| position: relative; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| padding: 0 tokens.bpk-spacing-md(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. It's parent element already set padding top/bottom, if add padding top/bottom here, will cause spacing much big than design. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| flex: 0 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| calc( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -52,6 +58,30 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
| overflow: visible; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| box-sizing: border-box; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| scroll-snap-align: start; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @include breakpoints.bpk-breakpoint-mobile { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // On mobile, if there is more than one card in the viewport, the last card only shows partially, so subtract 0.8 when do the calculation | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| flex: 0 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| calc( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| 100% - | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| tokens.bpk-spacing-md() * | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| (var(--initially-shown-cards, 3) - 1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) / | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| max(1, (var(--initially-shown-cards, 3) - 0.8)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Could you explain the key numbers in this code and provide a note to help others understand it for future iterations? 🙇♀️ |
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| &:first-child { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| padding-left: tokens.bpk-spacing-sm(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @include utils.bpk-rtl { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| padding-right: tokens.bpk-spacing-sm(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| padding-left: tokens.bpk-spacing-md(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,15 +16,23 @@ | |
| * limitations under the License. | ||
| */ | ||
| import type { CSSProperties } from 'react'; | ||
| import { | ||
| useRef, | ||
| useState, | ||
| useEffect, | ||
| isValidElement, | ||
| Children, | ||
| useMemo, | ||
| } from 'react'; | ||
|
|
||
| import throttle from 'lodash/throttle'; | ||
|
|
||
| import { cssModules } from '../../../bpk-react-utils'; | ||
|
|
||
| import { RENDER_BUFFER_SIZE } from './constants'; | ||
| import { | ||
| lockScroll, | ||
| setA11yTabIndex, | ||
| useScrollToCard, | ||
| useIntersectionObserver, | ||
| } from './utils'; | ||
|
|
@@ -44,7 +52,6 @@ const BpkCardListCarousel = (props: CardListCarouselProps) => { | |
| initiallyShownCards, | ||
| isMobile = false, | ||
| layout, | ||
| slideLabel = (index: number, childrenLength: number) => | ||
| `slide ${index + 1} of ${childrenLength}`, | ||
| } = props; | ||
|
|
@@ -59,7 +66,11 @@ const BpkCardListCarousel = (props: CardListCarouselProps) => { | |
|
|
||
| const childrenLength = Children.count(children); | ||
| const [root, setRoot] = useState<HTMLElement | null>(null); | ||
| const [, forceUpdate] = useState(0); | ||
| const cardRefs = useRef<Array<HTMLDivElement | null>>([]); | ||
| const hasBeenVisibleRef = useRef<Set<number>>(new Set()); | ||
| const firstCardWidthRef = useRef<number | null>(null); | ||
| const firstCardHeightRef = useRef<number | null>(null); | ||
|
|
||
| const [visibilityList, setVisibilityList] = useState<number[]>( | ||
| Array(childrenLength).fill(0), | ||
|
|
@@ -73,6 +84,56 @@ const BpkCardListCarousel = (props: CardListCarouselProps) => { | |
| setVisibilityList, | ||
| ); | ||
|
|
||
| useScrollToCard( | ||
| currentIndex * initiallyShownCards, | ||
| root, | ||
| cardRefs, | ||
| stateScrollingLockRef, | ||
| ); | ||
|
|
||
| // Similar to Virtual Scrolling to improve performance | ||
| const firstVisibleIndex = Math.max(0, visibilityList.indexOf(1)); | ||
| const lastVisibleIndex = firstVisibleIndex + initiallyShownCards - 1; | ||
| const renderList = useMemo( | ||
| () => | ||
| visibilityList.map((_, index) => | ||
| index >= firstVisibleIndex - RENDER_BUFFER_SIZE && | ||
| index <= lastVisibleIndex + RENDER_BUFFER_SIZE | ||
| ? 1 | ||
| : 0, | ||
| ), | ||
| [visibilityList, firstVisibleIndex, lastVisibleIndex], | ||
| ); | ||
|
|
||
| const cardRefFns = useMemo( | ||
| () => | ||
| Array(childrenLength) | ||
| .fill(null) | ||
| .map((_, i) => (el: HTMLDivElement | null) => { | ||
| cardRefs.current[i] = el; | ||
| observerVisibility(el, i); | ||
| setA11yTabIndex(el, i, visibilityList); | ||
| // record the first card's width and height when it becomes visible | ||
| if (el && visibilityList[i] === 0) { | ||
| hasBeenVisibleRef.current.add(i); | ||
| if (firstCardWidthRef.current == null && el.offsetWidth) { | ||
| firstCardWidthRef.current = el.offsetWidth; | ||
| } | ||
| if (firstCardHeightRef.current == null && el.offsetHeight) { | ||
| firstCardHeightRef.current = el.offsetHeight; | ||
| } | ||
| } | ||
| }), | ||
| [ | ||
| childrenLength, | ||
| observerVisibility, | ||
| visibilityList, | ||
| hasBeenVisibleRef, | ||
| firstCardWidthRef, | ||
| firstCardHeightRef, | ||
| ], | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| const container = root; | ||
| if (isMobile || !container) return undefined; | ||
|
|
@@ -93,25 +154,25 @@ const BpkCardListCarousel = (props: CardListCarouselProps) => { | |
| }; | ||
| }, [root]); | ||
|
|
||
| useEffect(() => { | ||
| // update hasBeenVisibleRef to include the range of cards that should be visible | ||
| const start = currentIndex * initiallyShownCards; | ||
| const end = start + initiallyShownCards + RENDER_BUFFER_SIZE; | ||
| for (let i = start; i < end && i < childrenLength; i += 1) { | ||
| hasBeenVisibleRef.current.add(i); | ||
| } | ||
| }, [currentIndex, initiallyShownCards, childrenLength]); | ||
|
|
||
| useEffect(() => { | ||
| const handleResize = throttle(() => { | ||
| firstCardWidthRef.current = null; | ||
| firstCardHeightRef.current = null; | ||
| forceUpdate((n) => n + 1); | ||
| }, 200); | ||
|
|
||
| window.addEventListener('resize', handleResize); | ||
| return () => window.removeEventListener('resize', handleResize); | ||
| }, []); | ||
|
|
||
| return ( | ||
| <div | ||
|
|
@@ -127,30 +188,43 @@ const BpkCardListCarousel = (props: CardListCarouselProps) => { | |
| {children.map((card, index) => { | ||
| if (!isValidElement(card)) return null; | ||
|
|
||
| const cardDimensionStyle: CSSProperties = {}; | ||
| if (firstCardWidthRef.current) { | ||
| cardDimensionStyle.width = `${firstCardWidthRef.current}px`; | ||
| } | ||
| if (firstCardHeightRef.current) { | ||
| cardDimensionStyle.height = `${firstCardHeightRef.current}px`; | ||
| } | ||
|
|
||
| // Only render cards that are within the renderList range or have been visible before | ||
| if (renderList[index] !== 1 && !hasBeenVisibleRef.current.has(index)) { | ||
|
There was a problem hiding this comment. If the current card has not entered the visible area, replace it with an empty element. The empty element should be consistent with the card width/height to avoid reflow. |
||
| return ( | ||
| <div | ||
| key={`placeholder-${index.toString()}`} | ||
| style={{ | ||
| ...shownNumberStyle, | ||
| ...cardDimensionStyle, | ||
| flexShrink: 0, | ||
| visibility: 'hidden', | ||
| }} | ||
| aria-hidden="true" | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <div | ||
| className={getClassName(`bpk-card-list-row-rail__${layout}__card`)} | ||
| ref={cardRefFns[index]} | ||
| style={{ | ||
| ...shownNumberStyle, | ||
| ...cardDimensionStyle, | ||
| }} | ||
| key={`carousel-card-${index.toString()}`} | ||
| role="group" | ||
| aria-label={slideLabel(index, childrenLength)} | ||
|
There was a problem hiding this comment. 🤔 why do we remove this? By the way, There was a problem hiding this comment. Since multiple cards can be displayed on one screen currently and these cards have no primary or secondary distinctions (for example, three cards are displayed on one screen and all three are equally important), the current aria-current is not accurate: it only marks the first one, but there are three "current" ones. If aria-current is added to all three actually displayed pictures, multiple "current" designations will also cause confusion. But love to discuss more. There was a problem hiding this comment. Removing this is ok, aria-current="page" can be used on the pagnation dots, but not on a single slide :D |
||
| > | ||
| {card} | ||
| </div> | ||
| ); | ||
| })} | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍