-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Simplify native attachment gallery paging context and improve code #34080
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
Changes from 37 commits
4ce49a3
79bfaa3
391d035
be3c674
30313f2
51fbe58
3c4efb9
934bfd3
6be9adb
a712b07
4d7c3e3
3376f86
7195fdf
c5669dd
d2424d8
2cb2e3c
e4d30dd
860cc19
d469354
5c11456
9f0c0ac
d08b392
500c5cd
08d9a40
f9cac81
ada4c98
c0b21fc
a03d476
d9460c4
57676b4
0ec7d19
390fb5c
6af0ba1
66ec3e6
a3eb193
96e72c9
5b9d413
577110a
d8d8ac9
39a65a6
62651a0
d738e43
cfc6174
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,26 @@ import {createContext} from 'react'; | |
import type PagerView from 'react-native-pager-view'; | ||
import type {SharedValue} from 'react-native-reanimated'; | ||
|
||
/** | ||
* The pager items array is used within the pager to render and navigate between the images | ||
*/ | ||
type AttachmentCarouselPagerItems = { | ||
/** The source of the image is used to identify each attachment/page in the pager */ | ||
source: string; | ||
|
||
/** The index of the pager item determines the order of the images in the pager */ | ||
index: number; | ||
/** | ||
* The active state of the pager item determines whether the image is currently transformable with pinch, pan and tap gestures | ||
*/ | ||
isActive: boolean; | ||
}; | ||
|
||
type AttachmentCarouselPagerContextValue = { | ||
/** The list of items that are shown in the pager */ | ||
pagerItems: AttachmentCarouselPagerItems[]; | ||
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. Please add comments for both of these props. |
||
/** The index of the active page */ | ||
pecanoro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
activePage: number; | ||
pagerRef: ForwardedRef<PagerView>; | ||
isPagerScrolling: SharedValue<boolean>; | ||
isScrollEnabled: SharedValue<boolean>; | ||
|
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,44 @@ 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. | ||||||||
*/ | ||||||||
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. There is a mix of comment styles in this PR. The most consistent pattern across the codebase to use is to keep it all on a single like like |
||||||||
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. | ||||||||
*/ | ||||||||
pecanoro marked this conversation as resolved.
Show resolved
Hide resolved
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 +67,14 @@ function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelecte | |||||||
}, []); | ||||||||
|
||||||||
useEffect(() => { | ||||||||
setActivePageState(initialIndex); | ||||||||
activePage.value = initialIndex; | ||||||||
}, [activePage, initialIndex]); | ||||||||
setActivePageIndex(initialPage); | ||||||||
activePage.value = initialPage; | ||||||||
}, [activePage, initialPage]); | ||||||||
|
||||||||
/** | ||||||||
* The pager uses the source index and current active state to render the pages. | ||||||||
*/ | ||||||||
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.
Suggested change
This isn't a valuable comment IMO. The code is clear enough. 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. As a general rule of thumb, code comments should explain why the logic is what it is, and comments that only explain what the code is doing are low-value. |
||||||||
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 +114,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 +143,38 @@ function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelecte | |||||||
[], | ||||||||
); | ||||||||
|
||||||||
const Content = useMemo( | ||||||||
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. Rename this to something more descriptive (and lower case the variable name). Suggestion: 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. Why is |
||||||||
() => | ||||||||
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> | ||||||||
)), | ||||||||
[activePageIndex, activeSource, items, styles.flex1], | ||||||||
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. NAB: |
||||||||
); | ||||||||
|
||||||||
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> | ||||||||
))} | ||||||||
{Content} | ||||||||
</AnimatedPagerView> | ||||||||
</AttachmentCarouselPagerContext.Provider> | ||||||||
); | ||||||||
|
Uh oh!
There was an error while loading. Please reload this page.