[native] introduce FullScreenViewModal
ClosedPublic
Actions

Authored by ginsu on Oct 4 2023, 11:21 AM.

Details

Reviewers

atul
inka
ashoat

Commits

rCOMM4bfd03b6a558: [native] introduce FullScreenViewModal

Summary

To view a full screen avatar for user profiles we are going to want to reuse parts of the ImageModal since it has all the same zooming/panning/gesture functionality we need. However, right now there are parts of ImageModal that are specific to viewing a multimedia message. We need to factor those parts out, so I created FullScreenViewModal which will be a generic component that can be used in both cases for image multimedia messages and user avatars.

Outlined below are the steps I will take in this stack (each point here will be it's own diff):

Introduce FullScreenViewModal file and move everything from the ImageModal file into here, and change what is absolutely necessary (the imports)
Correct the naming inside of FullScreenViewModal
Lift out rendering of <Multimedia /> in favor for a generic content child
Lift save content / copy content functionality out of FullScreenViewModal and make those optional props
Lift the imageDimensions function out of FullScreenViewModal and introduce a required generic contentDimesions prop

Test Plan

Confirmed that there were no regressions with the viewing an image multimedia message experience

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Oct 4 2023, 11:21 AM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptOct 4 2023, 11:21 AM

ginsu edited the summary of this revision. (Show Details)Oct 4 2023, 11:25 AM

ginsu edited the summary of this revision. (Show Details)Oct 4 2023, 11:35 AM

ginsu added reviewers: atul, inka.

Harbormaster completed remote builds in B22975: Diff 31640.Oct 4 2023, 11:39 AM

ginsu requested review of this revision.Oct 4 2023, 11:39 AM

ginsu added inline comments.Oct 4 2023, 11:40 AM

native/components/full-screen-view-modal.react.js
147–153 ↗	(On Diff #31640)	We want to keep these nav params in `ImageModal` since`ImageModal` is still the component we navigate to
native/media/image-modal.react.js
3 ↗	(On Diff #31640)	Created this gist so that is a bit more clear to see what's happening here. The only changes I am doing here are importing `FullScreenViewModal` and directly rendering it in `ImageModal` while passing the `navigation` and `route` param down.

ginsu added inline comments.Oct 4 2023, 11:43 AM

native/media/image-modal.react.js
3 ↗	(On Diff #31640)	while passing the navigation and route param down. *This should be props not param

ginsu added a child revision: D9361: [native] rename instances of ImageModal to FullScreenViewModal in FullScreenViewModal component.Oct 4 2023, 11:54 AM

We need to factor those parts out, so I created FullScreenViewModal which will be a generic component that can be used in both cases for image multimedia messages and user avatars.

Seems like a well thought out approach

Adding @ashoat as blocking since I don't have a ton of experience w/ Reanimated and had some issues w/ refactoring Reanimated code in the past (specifically https://phab.comm.dev/D9206#271158)

Adding @ashoat as blocking since I don't have a ton of experience w/ Reanimated and had some issues w/ refactoring Reanimated code in the past (specifically https://phab.comm.dev/D9206#271158)

TouchableWithoutFeedback is actually from React Native, not Reanimated. I don't think there's any need for ImageModal to forward any extra props to FullscreenImageModal. React Navigation renders screens, and it doesn't do anything weird like TouchableWithoutFeedback does.

This revision is now accepted and ready to land.Oct 4 2023, 6:13 PM

rebase before landing

Harbormaster completed remote builds in B22989: Diff 31656.Oct 4 2023, 8:20 PM

Closed by commit rCOMM4bfd03b6a558: [native] introduce FullScreenViewModal (authored by ginsu). · Explain WhyOct 4 2023, 8:50 PM

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM4bfd03b6a558: [native] introduce FullScreenViewModal.