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.

Sign up for GitHub

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

Jump to bottom

[TS migration] Migrate 'ImageView' component to TypeScript #34202

Merged
merged 16 commits into from
Jan 25, 2024
Merged

[TS migration] Migrate 'ImageView' component to TypeScript #34202

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
80f3867
fix: Migrate ImageView to ts
tienifr Jan 10, 2024
5ffd8e7
lint fix
tienifr Jan 10, 2024
a6b4682
type fix
tienifr Jan 15, 2024
3f19479
fix: revert native event
tienifr Jan 15, 2024
75810ae
Merge branch 'main' into fix/25113
tienifr Jan 17, 2024
f8bb83d
remove any type
tienifr Jan 17, 2024
e0c74d3
clean code
tienifr Jan 17, 2024
ea739c4
Merge branch 'main' into fix/25113
tienifr Jan 19, 2024
3e4c2fd
remove nativeEvent
tienifr Jan 19, 2024
df1502d
nativeEvent checker
tienifr Jan 19, 2024
dbadc31
lint fix
tienifr Jan 19, 2024
f2324c2
Merge branch 'main' into fix/25113
tienifr Jan 22, 2024
13c2a36
fix: clean code
tienifr Jan 22, 2024
4971bde
Merge branch 'main' into fix/25113
tienifr Jan 24, 2024
22f73e2
change to default export
tienifr Jan 25, 2024
bf30565
Merge branch 'main' into fix/25113
tienifr Jan 25, 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
52 changes: 0 additions & 52 deletions src/components/ImageView/index.native.js
View file
Open in desktop

This file was deleted.

39 changes: 39 additions & 0 deletions src/components/ImageView/index.native.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import React from 'react';
import Lightbox from '@components/Lightbox';
import {zoomRangeDefaultProps} from '@components/MultiGestureCanvas/propTypes';
import type {ImageViewProps} from './types';

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

return (
<Lightbox
source={url}
zoomRange={zoomRange}
isAuthTokenRequired={isAuthTokenRequired}
onScaleChanged={onScaleChanged}
onPress={onPress}
onError={onError}
index={carouselItemIndex}
activeIndex={carouselActiveItemIndex}
hasSiblingCarouselItems={hasSiblingCarouselItems}
style={style}
/>
);
}

ImageView.displayName = 'ImageView';

export default ImageView;
83 changes: 34 additions & 49 deletions src/components/ImageView/index.js → src/components/ImageView/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import React, {useCallback, useEffect, useRef, useState} from 'react';
import type {GestureResponderEvent, LayoutChangeEvent, NativeSyntheticEvent} from 'react-native';
import {View} from 'react-native';
import FullscreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import Image from '@components/Image';
import RESIZE_MODES from '@components/Image/resizeModes';
import PressableWithoutFeedback from '@components/Pressable/PressableWithoutFeedback';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import CONST from '@src/CONST';
import {imageViewDefaultProps, imageViewPropTypes} from './propTypes';
import viewRef from '@src/types/utils/viewRef';
import type {ImageLoadNativeEventData, ImageViewProps} from './types';

function ImageView({isAuthTokenRequired, url, fileName, onError}) {
type ZoomDelta = {offsetX: number; offsetY: number};

function ImageView({isAuthTokenRequired = false, url, fileName, onError}: ImageViewProps) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const [isLoading, setIsLoading] = useState(true);
Expand All @@ -25,29 +30,20 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {
const [imgWidth, setImgWidth] = useState(0);
const [imgHeight, setImgHeight] = useState(0);
const [zoomScale, setZoomScale] = useState(0);
const [zoomDelta, setZoomDelta] = useState({offsetX: 0, offsetY: 0});
const [zoomDelta, setZoomDelta] = useState<ZoomDelta>();

const scrollableRef = useRef(null);
const scrollableRef = useRef<HTMLDivElement>(null);
const canUseTouchScreen = DeviceCapabilities.canUseTouchScreen();

/**
* @param {Number} newContainerWidth
* @param {Number} newContainerHeight
* @param {Number} newImageWidth
* @param {Number} newImageHeight
*/
const setScale = (newContainerWidth, newContainerHeight, newImageWidth, newImageHeight) => {
const setScale = (newContainerWidth: number, newContainerHeight: number, newImageWidth: number, newImageHeight: number) => {
if (!newContainerWidth || !newImageWidth || !newContainerHeight || !newImageHeight) {
return;
}
const newZoomScale = Math.min(newContainerWidth / newImageWidth, newContainerHeight / newImageHeight);
setZoomScale(newZoomScale);
};

/**
* @param {SyntheticEvent} e
*/
const onContainerLayoutChanged = (e) => {
const onContainerLayoutChanged = (e: LayoutChangeEvent) => {
const {width, height} = e.nativeEvent.layout;
setScale(width, height, imgWidth, imgHeight);

Expand All @@ -57,10 +53,8 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {

/**
* When open image, set image width, height.
* @param {Number} imageWidth
* @param {Number} imageHeight
*/
const setImageRegion = (imageWidth, imageHeight) => {
const setImageRegion = (imageWidth: number, imageHeight: number) => {
tienifr marked this conversation as resolved.
Show resolved Hide resolved
if (imageHeight <= 0) {
return;
}
Expand All @@ -78,32 +72,29 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {
setIsZoomed(false);
};

const imageLoad = ({nativeEvent}) => {
const imageLoad = ({nativeEvent}: NativeSyntheticEvent<ImageLoadNativeEventData>) => {
setImageRegion(nativeEvent.width, nativeEvent.height);
setIsLoading(false);
};

/**
* @param {SyntheticEvent} e
*/
const onContainerPressIn = (e) => {
const onContainerPressIn = (e: GestureResponderEvent) => {
const {pageX, pageY} = e.nativeEvent;
setIsMouseDown(true);
setInitialX(pageX);
setInitialY(pageY);
setInitialScrollLeft(scrollableRef.current.scrollLeft);
setInitialScrollTop(scrollableRef.current.scrollTop);
setInitialScrollLeft(scrollableRef.current?.scrollLeft ?? 0);
setInitialScrollTop(scrollableRef.current?.scrollTop ?? 0);
};

/**
* Convert touch point to zoomed point
* @param {Boolean} x x point when click zoom
* @param {Boolean} y y point when click zoom
* @returns {Object} converted touch point
* @param x point when click zoom
* @param y point when click zoom
* @returns converted touch point
*/
const getScrollOffset = (x, y) => {
let offsetX;
let offsetY;
const getScrollOffset = (x: number, y: number) => {
tienifr marked this conversation as resolved.
Show resolved Hide resolved
let offsetX = 0;
let offsetY = 0;

// Container size bigger than clicked position offset
if (x <= containerWidth / 2) {
Expand All @@ -121,10 +112,8 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {
return {offsetX, offsetY};
};

/**
* @param {SyntheticEvent} e
*/
const onContainerPress = (e) => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const onContainerPress = (e: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The native event here is PointerEvent. Should we just convert the type for this case? I think we should bring this to slack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised here: https://expensify.slack.com/archives/C01GTK53T8Q/p1705315749638509

if (!isZoomed && !isDragging) {
if (e.nativeEvent) {
const {offsetX, offsetY} = e.nativeEvent;
Expand All @@ -148,13 +137,11 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {
}
};

/**
* @param {SyntheticEvent} e
*/
const trackPointerPosition = useCallback(
(e) => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(e: any) => {
// Whether the pointer is released inside the ImageView
const isInsideImageView = scrollableRef.current.contains(e.nativeEvent.target);
const isInsideImageView = scrollableRef.current?.contains(e.nativeEvent.target);

if (!isInsideImageView && isZoomed && isDragging && isMouseDown) {
setIsDragging(false);
Expand All @@ -165,14 +152,14 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {
);

const trackMovement = useCallback(
(e) => {
(e: MouseEvent) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here:

    const trackMovement = useCallback(
        (e: MouseEvent) => {
            const mouseEvent = e as unknown as ReactMouseEvent;
            if (!isZoomed) {
                return;
            }

            if (isDragging && isMouseDown && scrollableRef.current) {
                const x = mouseEvent.nativeEvent.x;
                const y = mouseEvent.nativeEvent.y;
                const moveX = initialX - x;
                const moveY = initialY - y;
                scrollableRef.current.scrollLeft = initialScrollLeft + moveX;
                scrollableRef.current.scrollTop = initialScrollTop + moveY;
            }

            setIsDragging(isMouseDown);
        },
        [initialScrollLeft, initialScrollTop, initialX, initialY, isDragging, isMouseDown, isZoomed],
    );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabioh8010 Thanks for your help. I just wonder can we remove nativeEvent here? As what I know, e.nativeEvent.x is the same as e.x in web/desktop platform. Fortunately, we're spliting ImageView into 2 files index.tsx and index.native.tsx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree your suggestion is better than any type, but it's quite workaround. If we know the reason behind using nativeEvent, we'll be all good with your idea ^

Copy link
Contributor

Choose a reason for hiding this comment

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

Well if it's well tested and it's working I believe you can change to remove nativeEvent, same for the other case const isInsideImageView = scrollableRef.current.contains(e.nativeEvent.target);.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this?

    const trackMovement = useCallback(
        (event: MouseEvent) => {
            if (!isZoomed) {
                return;
            }

            if (isDragging && isMouseDown && scrollableRef.current) {
                const x = event.x;
                const y = event.y;
                const moveX = initialX - x;
                const moveY = initialY - y;
                scrollableRef.current.scrollLeft = initialScrollLeft + moveX;
                scrollableRef.current.scrollTop = initialScrollTop + moveY;
            }

            setIsDragging(isMouseDown);
        },
        [initialScrollLeft, initialScrollTop, initialX, initialY, isDragging, isMouseDown, isZoomed],
    );

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the other function:

    const trackPointerPosition = useCallback(
        (event: MouseEvent) => {
            // Whether the pointer is released inside the ImageView
            const isInsideImageView = scrollableRef.current?.contains(event.target as Node);

            if (!isInsideImageView && isZoomed && isDragging && isMouseDown) {
                setIsDragging(false);
                setIsMouseDown(false);
            }
        },
        [isDragging, isMouseDown, isZoomed],
    );

if (!isZoomed) {
return;
}

if (isDragging && isMouseDown) {
const x = e.nativeEvent.x;
const y = e.nativeEvent.y;
if (isDragging && isMouseDown && scrollableRef.current) {
const x = e.x;
const y = e.y;
const moveX = initialX - x;
const moveY = initialY - y;
scrollableRef.current.scrollLeft = initialScrollLeft + moveX;
Expand Down Expand Up @@ -218,7 +205,7 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {
style={isLoading || zoomScale === 0 ? undefined : [styles.w100, styles.h100]}
// When Image dimensions are lower than the container boundary(zoomscale <= 1), use `contain` to render the image with natural dimensions.
// Both `center` and `contain` keeps the image centered on both x and y axis.
resizeMode={zoomScale > 1 ? Image.resizeMode.center : Image.resizeMode.contain}
resizeMode={zoomScale > 1 ? RESIZE_MODES.center : RESIZE_MODES.contain}
onLoadStart={imageLoadingStart}
onLoad={imageLoad}
onError={onError}
Expand All @@ -229,7 +216,7 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {
}
return (
<View
ref={scrollableRef}
ref={viewRef(scrollableRef)}
onLayout={onContainerLayoutChanged}
style={[styles.imageViewContainer, styles.overflowAuto, styles.pRelative]}
>
Expand All @@ -249,7 +236,7 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {
source={{uri: url}}
isAuthTokenRequired={isAuthTokenRequired}
style={[styles.h100, styles.w100]}
resizeMode={Image.resizeMode.contain}
resizeMode={RESIZE_MODES.contain}
onLoadStart={imageLoadingStart}
onLoad={imageLoad}
onError={onError}
Expand All @@ -261,8 +248,6 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) {
);
}

ImageView.propTypes = imageViewPropTypes;
ImageView.defaultProps = imageViewDefaultProps;
ImageView.displayName = 'ImageView';

export default ImageView;
46 changes: 0 additions & 46 deletions src/components/ImageView/propTypes.js
View file
Open in desktop

This file was deleted.

47 changes: 47 additions & 0 deletions src/components/ImageView/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import type {StyleProp, ViewStyle} from 'react-native';
import type {ZoomRange} from '@components/MultiGestureCanvas/types';

type ImageViewProps = {
/** Whether source url requires authentication */
isAuthTokenRequired?: boolean;

/** Handles scale changed event in image zoom component. Used on native only */
onScaleChanged: (scale: number) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to MultiGestureCanvas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MultiGestureCanvas can be out of scope


/** URL to full-sized image */
url: string;

/** image file name */
fileName: string;

/** 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;

/** Function for handle on press */
onPress?: () => void;

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

/** Range of zoom that can be applied to the content by pinching or double tapping. */
zoomRange?: ZoomRange;
};

type ImageLoadNativeEventData = {
width: number;
height: number;
};
tienifr marked this conversation as resolved.
Show resolved Hide resolved

export type {ImageViewProps, ImageLoadNativeEventData};
Loading
Loading