Page MenuHomePhabricator

[web] factor out mediaModalItem from FullScreenModal
ClosedPublic

Authored by ginsu on Oct 15 2023, 8:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jul 3, 2:02 AM
Unknown Object (File)
Tue, Jul 2, 2:31 PM
Unknown Object (File)
Sun, Jun 30, 5:51 PM
Unknown Object (File)
Sun, Jun 30, 1:47 PM
Unknown Object (File)
Sat, Jun 29, 7:35 PM
Unknown Object (File)
Sat, Jun 29, 4:08 AM
Unknown Object (File)
Sat, Jun 29, 4:08 AM
Unknown Object (File)
Sat, Jun 29, 4:08 AM
Subscribers

Details

Summary

In this diff, I factored out mediaModalItem from FullScreenViewModal and moved that logic into MultimediaModal. This diff also factors out the necessary styles, and the dimensions state which is used to help render mediaModalItem.

This is step 2 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. Lift the rendering of mediaModalItem out of FullScreenViewModal
  3. Finish renaming + updating remaining variables/callbacks to make FullScreenViewModal completly generic
  4. Introduce UserProfileAvatarModal

Part of this linear task: https://linear.app/comm/issue/ENG-5201/factor-out-viewing-image-multimedia-messages-specific-code-from

Depends on D9495

Test Plan

Confirmed that there were no regressions with viewing a multimedia message in a full screen view

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu added reviewers: atul, inka.
ginsu edited the summary of this revision. (Show Details)
ginsu requested review of this revision.Oct 15 2023, 9:20 PM
atul added inline comments.
web/media/multimedia-modal.react.js
52–112 ↗(On Diff #32010)

Not sure if it's worth addressing now, but I'd prefer breaking down this memoized block a bit so things are more readable.

113–121 ↗(On Diff #32010)

Let's memoize this guy.

web/modals/full-screen-view-modal.react.js
108 ↗(On Diff #32010)

Doesn't hurt to memoize

This revision is now accepted and ready to land.Oct 16 2023, 2:01 PM

address @atul's comments + rebase before landing

re requesting review just to confirm that I correctly memoized everything + addressed all of @atul's feedback

Thanks for addressing feedback

This revision is now accepted and ready to land.Oct 17 2023, 2:19 PM