[TS migration] Migrate 'ImageView' component to TypeScript #34202
Conversation
|
@s77rt Can you help review the PR? In this PR, I use type |
|
|
||
| /** 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; |
There was a problem hiding this comment.
Move this to MultiGestureCanvas
There was a problem hiding this comment.
MultiGestureCanvas can be out of scope
src/components/ImageView/index.tsx
Outdated
| const onContainerPress = (e) => { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const onContainerPress = (e: any) => { |
There was a problem hiding this comment.
The native event here is PointerEvent. Should we just convert the type for this case? I think we should bring this to slack
There was a problem hiding this comment.
src/components/ImageView/index.tsx
Outdated
| // Whether the pointer is released inside the ImageView | ||
| const isInsideImageView = scrollableRef.current.contains(e.nativeEvent.target); | ||
| const isInsideImageView = scrollableRef.current?.contains(e.target as Node); |
There was a problem hiding this comment.
Please test if this change won't create regressions
There was a problem hiding this comment.
I tested, e.nativeEvent.target is the same as e.target on web/desktop
There was a problem hiding this comment.
I can't find the best type for this so I just remove nativeEvent, I'm trying to find the reasonable type in this case
There was a problem hiding this comment.
One way to solve this:
First, add this ReactMouseEvent type to react import:
import React, {MouseEvent as ReactMouseEvent, useCallback, useEffect, useRef, useState} from 'react';Then, use it like this:
const trackPointerPosition = useCallback(
(e: MouseEvent) => {
const mouseEvent = e as unknown as ReactMouseEvent;
// Whether the pointer is released inside the ImageView
const isInsideImageView = scrollableRef.current?.contains(mouseEvent.nativeEvent.target as Node);
if (!isInsideImageView && isZoomed && isDragging && isMouseDown) {
setIsDragging(false);
setIsMouseDown(false);
}
},
[isDragging, isMouseDown, isZoomed],
);
|
@s77rt @fabioh8010 Thanks for all your helps. I updated my PR. |
|
@tienifr Please change the title to "[TS migration] Migrate 'ImageView' component to TypeScript" |
tienifr
changed the title
| // eslint-disable-next-line import/prefer-default-export | ||
| export type {ZoomRange}; |
There was a problem hiding this comment.
What's the reason for this change?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@tienifr did you forget to address this feedbakc?
src/components/ImageView/index.tsx
Outdated
| // Whether the pointer is released inside the ImageView | ||
| const isInsideImageView = scrollableRef.current.contains(e.nativeEvent.target); | ||
| const isInsideImageView = scrollableRef.current?.contains(e.target as Node); |
There was a problem hiding this comment.
One way to solve this:
First, add this ReactMouseEvent type to react import:
import React, {MouseEvent as ReactMouseEvent, useCallback, useEffect, useRef, useState} from 'react';Then, use it like this:
const trackPointerPosition = useCallback(
(e: MouseEvent) => {
const mouseEvent = e as unknown as ReactMouseEvent;
// Whether the pointer is released inside the ImageView
const isInsideImageView = scrollableRef.current?.contains(mouseEvent.nativeEvent.target as Node);
if (!isInsideImageView && isZoomed && isDragging && isMouseDown) {
setIsDragging(false);
setIsMouseDown(false);
}
},
[isDragging, isMouseDown, isZoomed],
);
src/components/ImageView/index.tsx
Outdated
| @@ -165,14 +140,14 @@ function ImageView({isAuthTokenRequired, url, fileName, onError}) { | |||
| ); | |||
|
|
|||
| const trackMovement = useCallback( | |||
| (e) => { | |||
| (e: MouseEvent) => { | |||
There was a problem hiding this comment.
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],
);There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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 ^
There was a problem hiding this comment.
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);.
There was a problem hiding this comment.
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],
);There was a problem hiding this comment.
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],
);
|
@tienifr Is this ready for review? I still see the |
|
type PressableOnPress = (event?: GestureResponderEvent | KeyboardEvent | SyntheticEvent<Element, PointerEvent>) => void | Promise<void>;
// ImageView/index.tsx
const onContainerPress: OnPress = (event) => {
if (!isZoomed && !isDragging) {
if (event && 'nativeEvent' in event && 'offsetX' in event.nativeEvent) {Please test it on each platform (console log the event), to make sure the type is correct, if needed adjust it. My understanding is:
|
Reviewer Checklist
Screenshots/VideosAndroid: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
|
|
@tienifr There is a failing test (Reassure Performance Tests) can you try merge main see if that fixes it? |
| // eslint-disable-next-line import/prefer-default-export | ||
| export type {ZoomRange}; |
There was a problem hiding this comment.
@tienifr did you forget to address this feedbakc?
|
@mountiny updated |
|
@tienifr Can you please sync with main too and retest quickly locally? |
|
Test works well @mountiny Screen.Recording.2024-01-25.at.18.18.35.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.33-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.33-5 🚀
|
Details
Fixed Issues
$ #25113
PROPOSAL: NA
Tests
Offline tests
Same as above
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2024-01-10.at.15.22.14.mov
Android: mWeb Chrome
Screen.Recording.2024-01-10.at.15.19.55.mov
iOS: Native
Screen.Recording.2024-01-10.at.12.04.24.mov
iOS: mWeb Safari
Screen.Recording.2024-01-10.at.11.51.54.mov
MacOS: Chrome / Safari
Screen.Recording.2024-01-10.at.11.49.20.mov
MacOS: Desktop
Screen.Recording.2024-01-10.at.11.56.35.mov