Page MenuHomePhabricator

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

Authored by rohan on Feb 23 2023, 10:16 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Unknown Object (File)
Thu, Nov 7, 11:38 PM
Unknown Object (File)
Thu, Nov 7, 7:31 PM
Unknown Object (File)
Tue, Oct 29, 5:50 PM
Unknown Object (File)
Tue, Oct 22, 9:22 AM
Subscribers

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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 <

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.
MultimediaModalModal
Screenshot 2023-02-23 at 5.10.04 PM.png (1×3 px, 5 MB)
Screenshot 2023-02-23 at 5.10.20 PM.png (1×3 px, 3 MB)
  • If media are different sizes, we get an inconsistent look based on the size of the image. For example:
MultimediaModal Small ImageMultimediaModal Normal Image
Screenshot 2023-02-23 at 5.12.51 PM.png (1×3 px, 1 MB)
Screenshot 2023-02-23 at 5.10.20 PM.png (1×3 px, 3 MB)

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

Modal Small ImageModal Normal Image
Screenshot 2023-02-23 at 5.14.56 PM.png (1×3 px, 2 MB)
Screenshot 2023-02-23 at 5.10.20 PM.png (1×3 px, 3 MB)

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

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:

  1. Use Modal like in this diff
  2. 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 edited the summary of this revision. (Show Details)
rohan edited the summary of this revision. (Show Details)

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)

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

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