Carousel : Add new options, and fixes / Add Story Indicator#200
Carousel : Add new options, and fixes / Add Story Indicator#200nparigi-ledger wants to merge 2 commits intomainfrom
Conversation
…m stories - Add new options to Carousel and fixes about delay and activeIndex with scroll
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
nparigi-ledger
changed the title
LFBarreto
left a comment
LFBarreto
left a comment
There was a problem hiding this comment.
Looking good overall but some polish remaining imo 😛
Also web native storybook is slightly broken on this
https://ledger-live-ui-native-git-feature-carousel-story-ledgerhq.vercel.app/?path=/story/carousel--customindicator-story&knob-autoDelay=4999&knob-restartAfterEnd=true&knob-scrollOnSidePress=true
| // Avoid conflict with styled-system's size property by nulling size and renaming it | ||
| size: undefined, | ||
| flexDirection: "row", | ||
| alignItems: "stretch", |
There was a problem hiding this comment.
i think you can use witdh: "100%" as a prop on Flex too
| useEffect(() => { | ||
| if (isActive) { | ||
| width.value = 100; | ||
| } else if (full) { |
There was a problem hiding this comment.
if (full || !IsActive)
so you dont repeat the if statement here
| () => ({ | ||
| width: withTiming(`${width.value}%`, { | ||
| duration: isActive ? duration || 200 : 0, | ||
| easing: duration ? Easing.linear : Easing.linear, |
| return ( | ||
| <Flex | ||
| height={4} | ||
| backgroundColor="neutral.c50" |
There was a problem hiding this comment.
maybe we want that color to be a prop for easier future integration 🤔
| height={4} | ||
| backgroundColor="neutral.c50" | ||
| margin={"auto"} | ||
| borderRadius={"8px"} |
There was a problem hiding this comment.
borderRadius uses radii styled system gradation i think this is equals to borderRadius={2} or something
| flex={1} | ||
| mx={1} | ||
| > | ||
| {full ? <ActiveBar /> : <AnimatedBar style={animatedStyles} />} |
There was a problem hiding this comment.
since the full state doesn't require any animation and is a simple switch here i would put all animated logic into a AnimatedBar component directly 🤔
| return ( | ||
| <TabsContainer> | ||
| {storiesArray.map((_, storyIndex) => ( | ||
| <StoryBar |
There was a problem hiding this comment.
probably add a key so react doesn't warn you about this
| scrollEventThrottle={200} | ||
| contentContainerStyle={{ width: `${fullWidth}%` }} | ||
| decelerationRate="fast" | ||
| disableIntervalMomentum={true} |
There was a problem hiding this comment.
just disableIntervalMomentum if its true
| onChange?: (index: number) => void; | ||
| duration?: number; | ||
| }> | ||
| | React.ReactElement; |
There was a problem hiding this comment.
not a big fan of mixed types, it works but it requires a probably costly check on React.isValidElement where you could just have an extra prop IndicatorElement
and check what to show
{
IndicatorElement ? (
IndicatorElement
) : IndicatorComponent
? (
<IndicatorComponent
...
/>
) : null
}
2 participants
This is non-breaking changes
Add StoryIndicator, an indicator for Carousel component like Instagram stories
Add new options to Carousel (mostly to be able to have a Carousel like Instagram stories, like having the possibility to tap on side to scroll, custom indicator, no restart after carousel end etc)
The delay was not resetting after scrolling
When scrolling to next item, the activeIndex was changing multiple time from the current index to the next index.
Screen_Recording_20220516-151401_Expo.Go.mp4