Page MenuHomePhabricator
Differential D6863

[web] Create a new modal for the media gallery on web
ClosedPublic
Actions

Authored by rohan on Feb 23 2023, 10:09 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 9:39 PM
Unknown Object (File)
Sat, Nov 23, 8:43 PM
Unknown Object (File)
Sat, Nov 23, 7:14 PM
Unknown Object (File)
Tue, Nov 12, 10:42 PM
Unknown Object (File)
Tue, Nov 12, 8:03 PM
Unknown Object (File)
Fri, Nov 8, 12:31 AM
Unknown Object (File)
Fri, Nov 8, 12:31 AM
Unknown Object (File)
Fri, Nov 8, 12:31 AM
View All Files
Subscribers
ashoat

Details

Reviewers
atul
ginsu
tomek
ashoat
Commits
rCOMM5596e52e5334: [web] Create a new modal for the media gallery on web
Summary

The web client needs a modal to contain all of the shared media for a specific thread. Much of the logic here is the same that is in the native counterpart, D6659.

Linear: https://linear.app/comm/issue/ENG-2878/create-a-new-modal-for-the-media-gallery-on-web

Depends on D6720

Test Plan

The next diff will add the media modal to the thread menu to click through into, but I will show a video of what the modal looks like on web below.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan created this revision.Feb 23 2023, 10:09 AM
Herald added a subscriber: ashoat. · View Herald TranscriptFeb 23 2023, 10:09 AM
rohan edited the test plan for this revision. (Show Details)Feb 23 2023, 10:10 AM
rohan attached a referenced file: F398935: Screen Recording 2023-02-23 at 1.06.31 PM.mov. (Show Details)
rohan added a child revision: D6864: [web] Link modal in the thread menu.Feb 23 2023, 10:12 AM
Harbormaster completed remote builds in B16841: Diff 23020.Feb 23 2023, 10:25 AM
rohan requested review of this revision.Feb 23 2023, 10:25 AM
ashoat added a comment.Feb 23 2023, 12:32 PM

Just some nits

web/modals/threads/gallery/thread-settings-media-gallery.react.js
59–73 ↗(On Diff #23020)

All of these return keywords (along with the curly braces) can be removed. When you have a function that just returns immediately, it's cleaner to skip the return

80–83 ↗(On Diff #23020)

I would reverse this condition and "early exit" so you can decrease indentation. More reading

rohan updated this revision to Diff 23037.Feb 23 2023, 1:57 PM

Reverse the handleScroll if condition to early return and format code

Harbormaster completed remote builds in B16858: Diff 23037.Feb 23 2023, 2:12 PM
ashoat accepted this revision.Feb 24 2023, 6:12 AM
ashoat added inline comments.
web/modals/threads/gallery/thread-settings-media-gallery.react.js
56

I would either replace this line with an invariant, or I would remove the condition on line 49 and let it default to this line

This revision is now accepted and ready to land.Feb 24 2023, 6:12 AM
rohan updated this revision to Diff 23079.Feb 24 2023, 1:53 PM

Rebase on D6485

rohan updated this revision to Diff 23080.Feb 24 2023, 1:55 PM

Address feedback on filteredMediaInfos

Harbormaster completed remote builds in B16897: Diff 23079.Feb 24 2023, 2:11 PM
Harbormaster completed remote builds in B16898: Diff 23080.Feb 24 2023, 2:15 PM
Closed by commit rCOMM5596e52e5334: [web] Create a new modal for the media gallery on web (authored by rohan). · Explain WhyFeb 28 2023, 2:05 PM
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMM5596e52e5334: [web] Create a new modal for the media gallery on web.

Revision Contents
Changeset List

PathSize
web/
modals/
threads/
gallery/
24 lines
128 lines

Diff 23037

View Options

web/modals/threads/gallery/thread-settings-media-gallery.css

Loading...
View Options

web/modals/threads/gallery/thread-settings-media-gallery.react.js

Loading...