Skip to content
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
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
4ce49a3
improve
Jan 7, 2024
79bfaa3
add callbacks
Jan 7, 2024
391d035
fix: typo
Jan 7, 2024
be3c674
move prop
Jan 7, 2024
30313f2
fix: wrong condition
Jan 7, 2024
51fbe58
pass pagerRef
Jan 7, 2024
3c4efb9
fix: arrow button visibility
Jan 7, 2024
934bfd3
pass pagerRef
Jan 7, 2024
6be9adb
fix: pager swiping
Jan 7, 2024
a712b07
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
Jan 8, 2024
4d7c3e3
Merge branch '@chrispader/restructure-multi-gesture-canvas' into @chr…
Jan 8, 2024
3376f86
generalize MultiGestureCanvas props
Jan 8, 2024
7195fdf
add more improvements to Lightbox
Jan 8, 2024
c5669dd
rename prop
Jan 8, 2024
d2424d8
Merge branch '@chrispader/restructure-multi-gesture-canvas' into @chr…
Jan 8, 2024
2cb2e3c
rename variable
Jan 8, 2024
e4d30dd
update prop types
Jan 8, 2024
860cc19
Merge branch '@chrispader/restructure-multi-gesture-canvas' into @chr…
Jan 8, 2024
d469354
fix: propType
Jan 8, 2024
5c11456
change to pagerRef
chrispader Jan 16, 2024
9f0c0ac
Merge branch 'main' into @chrispader/restructure-multi-gesture-canvas
Jan 29, 2024
d08b392
Merge branch '@chrispader/restructure-multi-gesture-canvas' into @chr…
Jan 29, 2024
500c5cd
fix: AttachmentView
Jan 29, 2024
08d9a40
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
Jan 29, 2024
f9cac81
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
Jan 29, 2024
ada4c98
rename context variable
Jan 29, 2024
c0b21fc
improve comments
Jan 29, 2024
a03d476
Update src/components/Lightbox/index.tsx
hannojg Feb 7, 2024
d9460c4
Update src/components/Attachments/AttachmentCarousel/Pager/Attachment…
hannojg Feb 7, 2024
57676b4
fix: prettier
kirillzyusko Feb 12, 2024
0ec7d19
refactor: setActivePageState -> setActivePageIndex, activePageState -…
kirillzyusko Feb 12, 2024
390fb5c
refactor: comment formatting
kirillzyusko Feb 12, 2024
6af0ba1
fix: strict comparison with null
kirillzyusko Feb 13, 2024
66ec3e6
refactor: rename handler
kirillzyusko Feb 13, 2024
a3eb193
fix: specify type for
kirillzyusko Feb 13, 2024
96e72c9
fix: comment about unused variables
kirillzyusko Feb 13, 2024
5b9d413
docs: added comments for new properties in context
kirillzyusko Feb 13, 2024
577110a
chore: added empty lines
kirillzyusko Feb 13, 2024
d8d8ac9
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
kirillzyusko Feb 14, 2024
39a65a6
Merge branch 'main' into @chrispader/simplify-native-attachment-carousel
kirillzyusko Feb 14, 2024
62651a0
fix: consistent comments
kirillzyusko Feb 14, 2024
d738e43
fix: better comment
kirillzyusko Feb 14, 2024
cfc6174
fix: remove useMemo
kirillzyusko Feb 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 1 addition & 14 deletions src/components/Attachments/AttachmentCarousel/CarouselItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,6 @@ const propTypes = {
transactionID: PropTypes.string,
}).isRequired,

/** Whether there is only one element in the attachment carousel */
isSingleItem: PropTypes.bool.isRequired,

/** The index of the carousel item */
index: PropTypes.number.isRequired,

/** The index of the currently active carousel item */
activeIndex: PropTypes.number.isRequired,

/** onPress callback */
onPress: PropTypes.func,
};
Expand All @@ -54,7 +45,7 @@ const defaultProps = {
onPress: undefined,
};

function CarouselItem({item, index, activeIndex, isSingleItem, onPress}) {
function CarouselItem({item, onPress}) {
const styles = useThemeStyles();
const {translate} = useLocalize();
const {isAttachmentHidden} = useContext(ReportAttachmentsContext);
Expand Down Expand Up @@ -104,10 +95,6 @@ function CarouselItem({item, index, activeIndex, isSingleItem, onPress}) {
source={item.source}
file={item.file}
isAuthTokenRequired={item.isAuthTokenRequired}
isUsedInCarousel
isSingleCarouselItem={isSingleItem}
carouselItemIndex={index}
carouselActiveItemIndex={activeIndex}
onPress={onPress}
transactionID={item.transactionID}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,27 @@ 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;
hannojg marked this conversation as resolved.
Show resolved Hide resolved
/**
pecanoro marked this conversation as resolved.
Show resolved Hide resolved
* The active state of the pager item determines whether the image is currently transformable with pinch, pan and tap gestures
*/
isActive: boolean;
};

type AttachmentCarouselPagerContextValue = {
pagerItems: AttachmentCarouselPagerItems[];
Copy link
Contributor

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.

activePage: number;
pagerRef: ForwardedRef<PagerView>;
isPagerScrolling: SharedValue<boolean>;
isScrollEnabled: SharedValue<boolean>;
Expand Down
82 changes: 58 additions & 24 deletions src/components/Attachments/AttachmentCarousel/Pager/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The 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 /** blah blah */. Can you please go through this PR and make sure all the prop comments match this style?

items: Attachment[];
/**
* The source of the currently active attachment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add more context to this comment to explain if it's a URL, a URI, or something else (maybe adding an example would help).

*/
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 [activePageState, setActivePageState] = useState(initialPage);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename these to:

  • activePageIndex
  • setActivePageIndex


const pageScrollHandler = usePageScrollHandler((e) => {
'worklet';
Expand All @@ -52,9 +67,14 @@ function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelecte
}, []);

useEffect(() => {
setActivePageState(initialIndex);
activePage.value = initialIndex;
}, [activePage, initialIndex]);
setActivePageState(initialPage);
activePage.value = initialPage;
}, [activePage, initialPage]);

/**
* The pager uses the source index and current active state to render the pages.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* The pager uses the source index and current active state to render the pages.
*/

This isn't a valuable comment IMO. The code is clear enough.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 === activePageState})), [activePageState, items]);

/**
* This callback is passed to the MultiGestureCanvas/Lightbox through the AttachmentCarouselPagerContext.
Expand Down Expand Up @@ -94,13 +114,15 @@ function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelecte

const contextValue = useMemo(
() => ({
pagerRef,
pagerItems,
activePage: activePageState,
isPagerScrolling,
isScrollEnabled,
pagerRef,
onTap: handleTap,
onScaleChanged: handleScaleChange,
}),
[isPagerScrolling, isScrollEnabled, handleTap, handleScaleChange],
[pagerItems, activePageState, isPagerScrolling, isScrollEnabled, handleTap, handleScaleChange],
);

const animatedProps = useAnimatedProps(() => ({
Expand All @@ -121,26 +143,38 @@ function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelecte
[],
);

const Content = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

The 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: carouselItems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is useMemo() being used here? Is there a problem with rendering the content inline down below like it was before?

() =>
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 === activePageState && activeSource === item.source}
/>
</View>
)),
[activePageState, activeSource, items, styles.flex1],
);

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>
);
Expand Down
23 changes: 2 additions & 21 deletions src/components/Attachments/AttachmentCarousel/index.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import variables from '@styles/variables';
import ONYXKEYS from '@src/ONYXKEYS';
import {defaultProps, propTypes} from './attachmentCarouselPropTypes';
import CarouselButtons from './CarouselButtons';
import CarouselItem from './CarouselItem';
import extractAttachmentsFromReport from './extractAttachmentsFromReport';
import AttachmentCarouselPager from './Pager';
import useCarouselArrows from './useCarouselArrows';
Expand Down Expand Up @@ -103,24 +102,6 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source,
[setShouldShowArrows],
);

/**
* Defines how a single attachment should be rendered
* @param {{ reportActionID: String, isAuthTokenRequired: Boolean, source: String, file: { name: String }, hasBeenFlagged: Boolean }} item
* @returns {JSX.Element}
*/
const renderItem = useCallback(
({item, index, isActive}) => (
<CarouselItem
item={item}
isSingleItem={attachments.length === 1}
index={index}
activeIndex={page}
isFocused={isActive && activeSource === item.source}
/>
),
[activeSource, attachments.length, page],
);

return (
<View style={[styles.flex1, styles.attachmentCarouselContainer]}>
{page == null ? (
Expand Down Expand Up @@ -148,8 +129,8 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source,

<AttachmentCarouselPager
items={attachments}
renderItem={renderItem}
initialIndex={page}
initialPage={page}
activeSource={activeSource}
onRequestToggleArrows={toggleArrows}
onPageSelected={({nativeEvent: {position: newPage}}) => updatePage(newPage)}
ref={pagerRef}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,7 @@ const propTypes = {
...withLocalizePropTypes,
};

function AttachmentViewImage({
url,
file,
isAuthTokenRequired,
isUsedInCarousel,
isSingleCarouselItem,
carouselItemIndex,
carouselActiveItemIndex,
isFocused,
loadComplete,
onPress,
onError,
isImage,
translate,
}) {
function AttachmentViewImage({url, file, isAuthTokenRequired, isFocused, loadComplete, onPress, onError, isImage, translate}) {
const styles = useThemeStyles();
const children = (
<ImageView
Expand All @@ -35,10 +21,6 @@ function AttachmentViewImage({
fileName={file.name}
isAuthTokenRequired={isImage && isAuthTokenRequired}
isFocused={isFocused}
isUsedInCarousel={isUsedInCarousel}
isSingleCarouselItem={isSingleCarouselItem}
carouselItemIndex={carouselItemIndex}
carouselActiveItemIndex={carouselActiveItemIndex}
/>
);

Expand Down
8 changes: 0 additions & 8 deletions src/components/Attachments/AttachmentView/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@ function AttachmentView({
translate,
isFocused,
isUsedInCarousel,
isSingleCarouselItem,
carouselItemIndex,
carouselActiveItemIndex,
isUsedInAttachmentModal,
isWorkspaceAvatar,
fallbackSource,
Expand Down Expand Up @@ -141,8 +138,6 @@ function AttachmentView({
isFocused={isFocused}
isAuthTokenRequired={isAuthTokenRequired}
encryptedSourceUrl={encryptedSourceUrl}
carouselItemIndex={carouselItemIndex}
carouselActiveItemIndex={carouselActiveItemIndex}
onPress={onPress}
onToggleKeyboard={onToggleKeyboard}
onLoadComplete={() => !loadComplete && setLoadComplete(true)}
Expand Down Expand Up @@ -171,9 +166,6 @@ function AttachmentView({
loadComplete={loadComplete}
isFocused={isFocused}
isUsedInCarousel={isUsedInCarousel}
isSingleCarouselItem={isSingleCarouselItem}
carouselItemIndex={carouselItemIndex}
carouselActiveItemIndex={carouselActiveItemIndex}
isImage={isImage}
onPress={onPress}
onError={() => {
Expand Down
17 changes: 1 addition & 16 deletions src/components/Attachments/AttachmentView/propTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,6 @@ const attachmentViewPropTypes = {
/** Whether this AttachmentView is shown as part of a AttachmentCarousel */
isUsedInCarousel: PropTypes.bool,

/** When "isUsedInCarousel" is set to true, determines whether there is only one item in the carousel */
isSingleCarouselItem: PropTypes.bool,

/** Whether this AttachmentView is shown as part of an AttachmentModal */
isUsedInAttachmentModal: PropTypes.bool,

/** The index of the carousel item */
carouselItemIndex: PropTypes.number,

/** The index of the currently active carousel item */
carouselActiveItemIndex: PropTypes.number,

/** Function for handle on press */
onPress: PropTypes.func,

Expand All @@ -39,11 +27,8 @@ const attachmentViewDefaultProps = {
name: '',
},
isFocused: false,
isUsedInCarousel: false,
isSingleCarouselItem: false,
carouselItemIndex: 0,
carouselActiveItemIndex: 0,
isSingleElement: false,
isUsedInCarousel: false,
isUsedInAttachmentModal: false,
onPress: undefined,
onScaleChanged: () => {},
Expand Down
17 changes: 1 addition & 16 deletions src/components/ImageView/index.native.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,13 @@ import Lightbox from '@components/Lightbox';
import {DEFAULT_ZOOM_RANGE} from '@components/MultiGestureCanvas';
import type {ImageViewProps} from './types';

function ImageView({
isAuthTokenRequired = false,
url,
style,
zoomRange = DEFAULT_ZOOM_RANGE,
onError,
isUsedInCarousel = false,
isSingleCarouselItem = false,
carouselItemIndex = 0,
carouselActiveItemIndex = 0,
}: ImageViewProps) {
const hasSiblingCarouselItems = isUsedInCarousel && !isSingleCarouselItem;

function ImageView({isAuthTokenRequired = false, url, style, zoomRange = DEFAULT_ZOOM_RANGE, onError}: ImageViewProps) {
return (
<Lightbox
uri={url}
zoomRange={zoomRange}
isAuthTokenRequired={isAuthTokenRequired}
onError={onError}
index={carouselItemIndex}
activeIndex={carouselActiveItemIndex}
hasSiblingCarouselItems={hasSiblingCarouselItems}
style={style}
/>
);
Expand Down
12 changes: 0 additions & 12 deletions src/components/ImageView/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,6 @@ type ImageViewProps = {
/** Handles errors while displaying the image */
onError?: () => void;

/** Whether this AttachmentView is shown as part of a AttachmentCarousel */
isUsedInCarousel?: boolean;

/** When "isUsedInCarousel" is set to true, determines whether there is only one item in the carousel */
isSingleCarouselItem?: boolean;

/** The index of the carousel item */
carouselItemIndex?: number;

/** The index of the currently active carousel item */
carouselActiveItemIndex?: number;

/** Additional styles to add to the component */
style?: StyleProp<ViewStyle>;

Expand Down
Loading
Loading