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

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

Merged
merged 16 commits into from
Jan 25, 2024
18 changes: 9 additions & 9 deletions src/components/ImageView/index.native.tsx
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
import React from 'react';
import Lightbox from '@components/Lightbox';
import {zoomRangeDefaultProps} from '@components/MultiGestureCanvas/propTypes';
import type * as ImageViewTypes from './types';
import type {ImageViewProps} from './types';

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

return (
Expand Down
26 changes: 19 additions & 7 deletions src/components/ImageView/index.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, {useCallback, useEffect, useRef, useState} from 'react';
import type {GestureResponderEvent, LayoutChangeEvent} from 'react-native';
import type {GestureResponderEvent, LayoutChangeEvent, NativeSyntheticEvent} from 'react-native';
import {View} from 'react-native';
import FullscreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import Image from '@components/Image';
Expand All @@ -10,9 +10,11 @@ import useThemeStyles from '@hooks/useThemeStyles';
import * as DeviceCapabilities from '@libs/DeviceCapabilities';
import CONST from '@src/CONST';
import viewRef from '@src/types/utils/viewRef';
import type * as ImageViewTypes from './types';
import type {ImageLoadNativeEventData, ImageViewProps} from './types';

function ImageView({isAuthTokenRequired = false, url, fileName, onError = () => {}}: ImageViewTypes.ImageViewProps) {
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 @@ -28,7 +30,7 @@ function ImageView({isAuthTokenRequired = false, 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<HTMLDivElement>(null);
const canUseTouchScreen = DeviceCapabilities.canUseTouchScreen();
Expand All @@ -49,6 +51,9 @@ function ImageView({isAuthTokenRequired = false, url, fileName, onError = () =>
setContainerWidth(width);
};

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

const imageLoad = ({nativeEvent}: {nativeEvent: ImageViewTypes.ImageLoadNativeEventData}) => {
const imageLoad = ({nativeEvent}: NativeSyntheticEvent<ImageLoadNativeEventData>) => {
setImageRegion(nativeEvent.width, nativeEvent.height);
setIsLoading(false);
};
Expand All @@ -81,6 +86,12 @@ function ImageView({isAuthTokenRequired = false, url, fileName, onError = () =>
setInitialScrollTop(scrollableRef.current?.scrollTop ?? 0);
};

/**
* Convert touch point to zoomed point
* @param x point when click zoom
* @param y point when click zoom
* @returns converted touch point
*/
const getScrollOffset = (x: number, y: number) => {
tienifr marked this conversation as resolved.
Show resolved Hide resolved
let offsetX = 0;
let offsetY = 0;
Expand Down Expand Up @@ -127,9 +138,10 @@ function ImageView({isAuthTokenRequired = false, url, fileName, onError = () =>
};

const trackPointerPosition = useCallback(
(e: MouseEvent) => {
// 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.target as Node);
const isInsideImageView = scrollableRef.current?.contains(e.nativeEvent.target);

if (!isInsideImageView && isZoomed && isDragging && isMouseDown) {
setIsDragging(false);
Expand Down
7 changes: 2 additions & 5 deletions src/components/ImageView/types.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import type {StyleProp, ViewStyle} from 'react-native';
import type ZoomRange from '@components/MultiGestureCanvas/types';
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 */
// eslint-disable-next-line react/no-unused-prop-types
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 */
Expand All @@ -18,9 +17,6 @@ type ImageViewProps = {
/** Handles errors while displaying the image */
onError?: () => void;

/** Whether this view is the active screen */
isFocused?: boolean;

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

Expand All @@ -47,4 +43,5 @@ type ImageLoadNativeEventData = {
width: number;
height: number;
};
tienifr marked this conversation as resolved.
Show resolved Hide resolved

export type {ImageViewProps, ImageLoadNativeEventData};
3 changes: 2 additions & 1 deletion src/components/Lightbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
import React, {useCallback, useEffect, useMemo, useState} from 'react';
import {ActivityIndicator, PixelRatio, StyleSheet, View} from 'react-native';
import useStyleUtils from '@hooks/useStyleUtils';
import stylePropTypes from '@styles/stylePropTypes';
import * as AttachmentsPropTypes from './Attachments/propTypes';
import Image from './Image';
import MultiGestureCanvas from './MultiGestureCanvas';
Expand Down Expand Up @@ -44,7 +45,7 @@ const propTypes = {
activeIndex: PropTypes.number,

/** Additional styles to add to the component */
style: PropTypes.oneOfType([PropTypes.bool, PropTypes.arrayOf(PropTypes.object), PropTypes.object]),
style: stylePropTypes,
};

const defaultProps = {
Expand Down
3 changes: 2 additions & 1 deletion src/components/MultiGestureCanvas/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ type ZoomRange = {
max: number;
};

export default ZoomRange;
// eslint-disable-next-line import/prefer-default-export
export type {ZoomRange};
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even though we just export one type, but I believe we shouldn't use export default here, because we can have many types in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, but in case we have more types we just change from default export to named ones, and do a refactor, wouldn't be such a big problem to do 🙂 I would prefer using a default export and removing this error supression

Copy link
Contributor

Choose a reason for hiding this comment

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

@tienifr did you forget to address this feedbakc?

2 changes: 1 addition & 1 deletion src/styles/stylePropTypes.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import PropTypes from 'prop-types';

const stylePropTypes = PropTypes.oneOfType([PropTypes.object, PropTypes.arrayOf(PropTypes.object), PropTypes.func]);
const stylePropTypes = PropTypes.oneOfType([PropTypes.object, PropTypes.bool, PropTypes.arrayOf(PropTypes.object), PropTypes.func]);

export default stylePropTypes;
Loading