Page MenuHomePhabricator

[native] introduce saveContentCallback and copyContentCallback props to full screen view modal
ClosedPublic

Authored by ginsu on Oct 4 2023, 12:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 27, 12:19 AM
Unknown Object (File)
Oct 15 2024, 10:26 AM
Unknown Object (File)
Oct 6 2024, 12:20 AM
Unknown Object (File)
Oct 5 2024, 10:23 PM
Unknown Object (File)
Oct 1 2024, 9:05 PM
Unknown Object (File)
Oct 1 2024, 9:05 PM
Unknown Object (File)
Oct 1 2024, 9:05 PM
Unknown Object (File)
Oct 1 2024, 9:05 PM
Subscribers

Details

Summary

This diff factors out the save and copy functions in FullScreenViewModal. Both of these functions are feature specific to image multimedia messages (since they are using things like MediaInfo and ChatMultimediaMessageInfoItem types that are not relevant to user avatars). I also think the concept of saving and copying someone's user avatar is a bit strange, so I felt like it was best just to make this functionality optional in general (if we wanted to in the future we can build out these save/copy callbacks for user avatars as a follow-up). Having these callbacks be optional means that we won't render the save or copy button unless these callbacks are passed down.

This is step 4 in the list below

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

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

Depends on D9359

Test Plan

Confirmed that copy and save functionality still works as expected. Also tested to see what happens when we don't pass in saveContentCallback and copyContentCallback (User avatars won't pass these props in and this is what I got):

Passed in saveContentCallback and copyContentCallback

Screenshot 2023-10-04 at 4.11.05 PM.png (1×1 px, 669 KB)

Omitted saveContentCallback and copyContentCallback:

Screenshot 2023-10-04 at 4.14.46 PM.png (1×1 px, 978 KB)

Screenshot 2023-10-04 at 4.14.37 PM.png (1×1 px, 836 KB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu requested review of this revision.Oct 4 2023, 1:28 PM

Makes sense, wonder if there's a way to make FullScreenViewModal a little more "children-agnostic" since saveContentCallback and copyContentCallback are kind of media-specific and presumably FullScreenViewModal should be a general purpose component.

Accepting to unblock, but might be worth thinking through a more general purpose API?

This revision is now accepted and ready to land.Oct 4 2023, 5:13 PM
In D9362#275121, @atul wrote:

Makes sense, wonder if there's a way to make FullScreenViewModal a little more "children-agnostic" since saveContentCallback and copyContentCallback are kind of media-specific and presumably FullScreenViewModal should be a general purpose component.

Accepting to unblock, but might be worth thinking through a more general purpose API?

Agree that there is room to improve to make this component more general purpose; however, I think that there is a lot of risk with touching/lifting the animated styles/animation logic, especially while its still in reanimated 1. Once we migrate this code to reanimated 2/3, I think then that would be a good point to revisit this. I will create a Linear task to track this