Paths

Table of Contentst

[web] Allow media gallery items to click through to a full screen view on web
ClosedPublic
Actions

Authored by rohan on Feb 23 2023, 10:16 AM.

Details

Reviewers

atul
ginsu
tomek

Commits

rCOMM82162de510a5: [web] Allow media gallery items to click through to a full screen view on web

Summary

The user should be able to click on any item in the media gallery modal and navigate to a larger view of the media content.

The designs are viewable on Figma.

Linear: https://linear.app/comm/issue/ENG-3016/allow-media-gallery-items-to-click-through-to-a-full-screen-view-on

I've created a separate task to track the restyling of MultimediaModal as I'm currently discussing it with Ted in the Design task for the media gallery.

Depends on D6864

Test Plan

Confirm that media items can be clicked through into a larger view.

Edit: New look after using the existing MultimediaModal:

Diff Detail

Repository

rCOMM Comm

Branch

media_gallery

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Feb 23 2023, 10:16 AM

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 23 2023, 10:16 AM

rohan edited the test plan for this revision. (Show Details)Feb 23 2023, 10:17 AM

rohan attached a referenced file: F398998: Screen Recording 2023-02-23 at 1.15.51 PM.mov. (Show Details)

rohan added inline comments.

web/modals/modal.react.js
44–45 ↗	(On Diff #23022)	I made the change to the existing Modal component to allow for a 'transparent' background since I needed one to match the media gallery designs, and instead of essentially copy-pasting the component into a new Modal, I thought it would be best to add support in the existing modal for it.

Harbormaster completed remote builds in B16843: Diff 23022.Feb 23 2023, 10:31 AM

rohan requested review of this revision.Feb 23 2023, 10:31 AM

ashoat added inline comments.Feb 23 2023, 12:37 PM

web/modals/threads/gallery/thread-settings-media-gallery-item.react.js
19 ↗	(On Diff #23022)	Weird that we have an empty string here. Maybe `Modal`'s `name` prop should be optional instead, and default to the empty string internally?
41 ↗	(On Diff #23022)	`MultimediaModal` doesn't seem to use the `Modal` component at all, and that's the closest comparison point to this work. What benefits do we get from using `Modal`? Is it really appropriate for us here, given that we have to add the `transparent` prop and set the name to the empty string?
50 ↗	(On Diff #23022)	We should probably use a real icon for the `<`

rohan added inline comments.Feb 23 2023, 2:17 PM

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

41 ↗

(On Diff #23022)

I went with Modal for two reasons:

Following the designs, and after speaking with Ted last week or two weeks ago, I asked if we'd want to use the existing full screen media view or the full screen view in the designs, and he confirmed that it'd be best to follow the designs so I used Modal to achieve this effect.

MultimediaModal	Modal

If media are different sizes, we get an inconsistent look based on the size of the image. For example:

MultimediaModal Small Image	MultimediaModal Normal Image

As opposed to using Modal, there's a level of consistency between media

Modal Small Image	Modal Normal Image

rohan attached a referenced file: F399486: Screenshot 2023-02-23 at 5.10.04 PM.png. (Show Details)Feb 23 2023, 2:17 PM

rohan attached a referenced file: F399514: Screenshot 2023-02-23 at 5.14.56 PM.png. (Show Details)

rohan attached a referenced file: F399487: Screenshot 2023-02-23 at 5.10.20 PM.png. (Show Details)

rohan attached a referenced file: F399488: Screenshot 2023-02-23 at 5.12.51 PM.png. (Show Details)

Why are we using Modal though? Should we just update MultimediaModal to look like those designs?

In D6865#204399, @ashoat wrote:

Why are we using Modal though? Should we just update MultimediaModal to look like those designs?

There's no strong reason besides the designs. We could do one of two things:

Use Modal like in this diff
Update MultimediaModal to support videos since I think it only supports images and also update it's styles to try to match the Figma designs, then use it here.

#2 is a little more involved - I can switch to using MultimediaModal and start a discussion on the linear design task to see if we're using MultimediaModal, if we want to use the same styles or restyle the modal entirely

Use MultimediaModal and allow video support for the modal

rohan added 1 blocking reviewer(s): atul.Feb 26 2023, 4:16 PM

Revert Modal changes

Harbormaster completed remote builds in B16918: Diff 23124.Feb 26 2023, 4:31 PM

Harbormaster completed remote builds in B16919: Diff 23125.Feb 26 2023, 4:35 PM

rohan edited the test plan for this revision. (Show Details)Feb 26 2023, 6:46 PM

rohan attached a referenced file: F402973: Screen Recording 2023-02-26 at 9.44.39 PM.mov. (Show Details)

rohan edited the summary of this revision. (Show Details)Feb 27 2023, 3:51 PM

rohan edited the summary of this revision. (Show Details)

rohan edited the summary of this revision. (Show Details)Feb 28 2023, 2:07 PM

Hey @rohan, you seem to have partially landed your stack. Just wanted to get some clarification – what is the current state of master after your partial land? I want to make sure we aren't in a bad state where we are blocked from deploys. (If so, we should revert)

In D6865#205681, @ashoat wrote:

Hey @rohan, you seem to have partially landed your stack. Just wanted to get some clarification – what is the current state of master after your partial land? I want to make sure we aren't in a bad state where we are blocked from deploys. (If so, we should revert)

Yeah there's no bad state whatsoever, the media gallery will be accessible via the thread menu. This diff just handles clicking through into a full screen experience, so the master branch just doesn't support onClick for the media gallery items until this is landed. I ran web locally checked out on master to confirm

atul accepted this revision.Mar 2 2023, 8:48 PM

This revision is now accepted and ready to land.Mar 2 2023, 8:48 PM

Closed by commit rCOMM82162de510a5: [web] Allow media gallery items to click through to a full screen view on web (authored by rohan). · Explain WhyMar 3 2023, 4:34 AM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM82162de510a5: [web] Allow media gallery items to click through to a full screen view on web.