-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify native attachment gallery paging context and improve code #34080
Merged
tgolen
merged 43 commits into
Expensify:main
from
margelo:@chrispader/simplify-native-attachment-carousel
Feb 14, 2024
Merged
Changes from all commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
4ce49a3
improve
79bfaa3
add callbacks
391d035
fix: typo
be3c674
move prop
30313f2
fix: wrong condition
51fbe58
pass pagerRef
3c4efb9
fix: arrow button visibility
934bfd3
pass pagerRef
6be9adb
fix: pager swiping
a712b07
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
4d7c3e3
Merge branch '@chrispader/restructure-multi-gesture-canvas' into @chr…
3376f86
generalize MultiGestureCanvas props
7195fdf
add more improvements to Lightbox
c5669dd
rename prop
d2424d8
Merge branch '@chrispader/restructure-multi-gesture-canvas' into @chr…
2cb2e3c
rename variable
e4d30dd
update prop types
860cc19
Merge branch '@chrispader/restructure-multi-gesture-canvas' into @chr…
d469354
fix: propType
5c11456
change to pagerRef
chrispader 9f0c0ac
Merge branch 'main' into @chrispader/restructure-multi-gesture-canvas
d08b392
Merge branch '@chrispader/restructure-multi-gesture-canvas' into @chr…
500c5cd
fix: AttachmentView
08d9a40
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
f9cac81
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
ada4c98
rename context variable
c0b21fc
improve comments
a03d476
Update src/components/Lightbox/index.tsx
hannojg d9460c4
Update src/components/Attachments/AttachmentCarousel/Pager/Attachment…
hannojg 57676b4
fix: prettier
kirillzyusko 0ec7d19
refactor: setActivePageState -> setActivePageIndex, activePageState -…
kirillzyusko 390fb5c
refactor: comment formatting
kirillzyusko 6af0ba1
fix: strict comparison with null
kirillzyusko 66ec3e6
refactor: rename handler
kirillzyusko a3eb193
fix: specify type for
kirillzyusko 96e72c9
fix: comment about unused variables
kirillzyusko 5b9d413
docs: added comments for new properties in context
kirillzyusko 577110a
chore: added empty lines
kirillzyusko d8d8ac9
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
kirillzyusko 39a65a6
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
kirillzyusko 62651a0
fix: consistent comments
kirillzyusko d738e43
fix: better comment
kirillzyusko cfc6174
fix: remove useMemo
kirillzyusko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import {createNativeWrapper} from 'react-native-gesture-handler'; | |
import type {PagerViewProps} from 'react-native-pager-view'; | ||
import PagerView from 'react-native-pager-view'; | ||
import Animated, {useAnimatedProps, useSharedValue} from 'react-native-reanimated'; | ||
import CarouselItem from '@components/Attachments/AttachmentCarousel/CarouselItem'; | ||
import useThemeStyles from '@hooks/useThemeStyles'; | ||
import AttachmentCarouselPagerContext from './AttachmentCarouselPagerContext'; | ||
import usePageScrollHandler from './usePageScrollHandler'; | ||
|
@@ -19,30 +20,40 @@ type AttachmentCarouselPagerHandle = { | |
setPage: (selectedPage: number) => void; | ||
}; | ||
|
||
type PagerItem = { | ||
key: string; | ||
url: string; | ||
type Attachment = { | ||
source: string; | ||
}; | ||
|
||
type AttachmentCarouselPagerProps = { | ||
items: PagerItem[]; | ||
renderItem: (props: {item: PagerItem; index: number; isActive: boolean}) => React.ReactNode; | ||
initialIndex: number; | ||
/** The attachments to be rendered in the pager. */ | ||
items: Attachment[]; | ||
|
||
/** The source (URL) of the currently active attachment. */ | ||
activeSource: string; | ||
|
||
/** The index of the initial page to be rendered. */ | ||
initialPage: number; | ||
|
||
/** A callback to be called when the page is changed. */ | ||
onPageSelected: () => void; | ||
|
||
/** | ||
* A callback that can be used to toggle the attachment carousel arrows, when the scale of the image changes. | ||
* @param showArrows If set, it will show/hide the arrows. If not set, it will toggle the arrows. | ||
*/ | ||
Comment on lines
+40
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI this is fine as a multi-line comment since it has a param description. |
||
onRequestToggleArrows: (showArrows?: boolean) => void; | ||
}; | ||
|
||
function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelected, onRequestToggleArrows}: AttachmentCarouselPagerProps, ref: ForwardedRef<AttachmentCarouselPagerHandle>) { | ||
function AttachmentCarouselPager({items, activeSource, initialPage, onPageSelected, onRequestToggleArrows}: AttachmentCarouselPagerProps, ref: ForwardedRef<AttachmentCarouselPagerHandle>) { | ||
const styles = useThemeStyles(); | ||
const pagerRef = useRef<PagerView>(null); | ||
|
||
const scale = useRef(1); | ||
const isPagerScrolling = useSharedValue(false); | ||
const isScrollEnabled = useSharedValue(true); | ||
|
||
const activePage = useSharedValue(initialIndex); | ||
const [activePageState, setActivePageState] = useState(initialIndex); | ||
const activePage = useSharedValue(initialPage); | ||
const [activePageIndex, setActivePageIndex] = useState(initialPage); | ||
|
||
const pageScrollHandler = usePageScrollHandler((e) => { | ||
'worklet'; | ||
|
@@ -52,9 +63,12 @@ function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelecte | |
}, []); | ||
|
||
useEffect(() => { | ||
setActivePageState(initialIndex); | ||
activePage.value = initialIndex; | ||
}, [activePage, initialIndex]); | ||
setActivePageIndex(initialPage); | ||
activePage.value = initialPage; | ||
}, [activePage, initialPage]); | ||
|
||
/** The `pagerItems` object that passed down to the context. Later used to detect current page, whether it's a single image gallery etc. */ | ||
const pagerItems = useMemo(() => items.map((item, index) => ({source: item.source, index, isActive: index === activePageIndex})), [activePageIndex, items]); | ||
|
||
/** | ||
* This callback is passed to the MultiGestureCanvas/Lightbox through the AttachmentCarouselPagerContext. | ||
|
@@ -94,13 +108,15 @@ function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelecte | |
|
||
const contextValue = useMemo( | ||
() => ({ | ||
pagerRef, | ||
pagerItems, | ||
activePage: activePageIndex, | ||
isPagerScrolling, | ||
isScrollEnabled, | ||
pagerRef, | ||
onTap: handleTap, | ||
onScaleChanged: handleScaleChange, | ||
}), | ||
[isPagerScrolling, isScrollEnabled, handleTap, handleScaleChange], | ||
[pagerItems, activePageIndex, isPagerScrolling, isScrollEnabled, handleTap, handleScaleChange], | ||
); | ||
|
||
const animatedProps = useAnimatedProps(() => ({ | ||
|
@@ -121,26 +137,34 @@ function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelecte | |
[], | ||
); | ||
|
||
const carouselItems = items.map((item, index) => ( | ||
<View | ||
key={item.source} | ||
style={styles.flex1} | ||
> | ||
<CarouselItem | ||
// @ts-expect-error TODO: Remove this once AttachmentView (https://github.com/Expensify/App/issues/25150) is migrated to TypeScript. | ||
item={item} | ||
isSingleItem={items.length === 1} | ||
index={index} | ||
isFocused={index === activePageIndex && activeSource === item.source} | ||
/> | ||
</View> | ||
)); | ||
|
||
return ( | ||
<AttachmentCarouselPagerContext.Provider value={contextValue}> | ||
<AnimatedPagerView | ||
pageMargin={40} | ||
offscreenPageLimit={1} | ||
onPageScroll={pageScrollHandler} | ||
onPageSelected={onPageSelected} | ||
ref={pagerRef} | ||
style={styles.flex1} | ||
initialPage={initialIndex} | ||
initialPage={initialPage} | ||
animatedProps={animatedProps} | ||
ref={pagerRef} | ||
> | ||
{items.map((item, index) => ( | ||
<View | ||
key={item.source} | ||
style={styles.flex1} | ||
> | ||
{renderItem({item, index, isActive: index === activePageState})} | ||
</View> | ||
))} | ||
{carouselItems} | ||
</AnimatedPagerView> | ||
</AttachmentCarouselPagerContext.Provider> | ||
); | ||
|
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
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
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
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
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
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
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.
Please add comments for both of these props.