Carousel: Groups #32024
Closed
Carousel: Groups #32024
Conversation
This file contains 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
📊 Bundle size report✅ No changes found |
Mitch-At-Work
force-pushed
the
user/mifraser/enable-embla-grouping
branch
from
1b70a8a
to
ed96b90
Compare
Mitch-At-Work
force-pushed
the
user/mifraser/enable-embla-grouping
branch
from
ed96b90
to
c302937
Compare
mltejera
reviewed
Jul 23, 2024
|
|
||
| /** | ||
| * Controls whether the scrollView will trim whitespace in order to position trailing items via alignment | ||
| * Default: false. |
There was a problem hiding this comment.
nit: use the jsdoc notation
| /** | ||
| * Group size enables pagination of multiple cards as a single slide | ||
| */ | ||
| groupSize?: number; |
There was a problem hiding this comment.
do we want to try and handle cases where a user (likely accidentally) inputs non positive or non-whole numbers?
Comment on lines
+15
to
+35
| if (groupSize) { | ||
| const groupValues: string[][] = []; | ||
| let groupIndex = -1; | ||
| values.forEach((value, index) => { | ||
| if (index % groupSize === 0) { | ||
| groupIndex++; | ||
| groupValues[groupIndex] = []; | ||
| } | ||
| groupValues[groupIndex].push(value); | ||
| }); | ||
|
|
||
| return ( | ||
| <state.root> | ||
| {groupValues.map(value => ( | ||
| <CarouselNavContextProvider value={value} key={value.join('-')}> | ||
| {renderNavButton(value)} | ||
| </CarouselNavContextProvider> | ||
| ))} | ||
| </state.root> | ||
| ); | ||
| } |
There was a problem hiding this comment.
There should be no computations in render functions
Comment on lines
+188
to
+190
| setStartIndex(index); | ||
| // Sets the nav items | ||
| setValue(values[cardIndex]); |
There was a problem hiding this comment.
This is going to cause a re-render after initial mount :(
| @@ -165,10 +175,21 @@ export function useCarousel_unstable(props: CarouselProps, ref: React.Ref<HTMLDi | |||
| }); | |||
|
|
|||
| useIsomorphicLayoutEffect(() => { | |||
| if (isFirstMount && value) { | |||
| scrollToValue(value, true); | |||
|
Closed in favor of: #32113 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previous Behavior
Users could place multiple items in the carousel view, with one navigation item per item.
Users could have multiple items within a single CarouselCard, but would require custom logic that would separate the visual cards from the actual carousel slides - this will cause future issues with our focus and accessibility implementations, so we need a way to group internally via carousel hooks.
New Behavior
GroupSize variable enables users to define groups of elements as a single navigation point, without having to build custom logic to place items in a single card.
Carousel nav buttons will now take a list of items, and will use the first index as the navigation points.
We ensure that the first item is the active value so that movement for next/previous is consistent every time.