Details

Reviewers

atul
ginsu
kamil
tomek
ashoat

Commits

rCOMM6e2f92634d55: [native] Render the media gallery in the thread settings on native

Summary

This diff handles actually displaying the media gallery component from the previous diff into the thread settings on native.

https://linear.app/comm/issue/ENG-2869/render-the-media-gallery-in-the-thread-settings-on-native

Depends on D6466

Test Plan

As mentioned in the previous diff, I've hard coded some images into the component to check how the component renders with images. Displayed below is the screenshot.

Render the media gallery in the thread settings on native .png (2×1 px, 1 MB)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Jan 30 2023, 8:46 AM

Herald added a subscriber: ashoat. · View Herald TranscriptJan 30 2023, 8:46 AM

rohan edited the summary of this revision. (Show Details)Jan 30 2023, 8:47 AM

rohan edited the test plan for this revision. (Show Details)

rohan attached a referenced file: F352330: Render the media gallery in the thread settings on native .png. (Show Details)

Harbormaster completed remote builds in B15853: Diff 21620.Jan 30 2023, 9:01 AM

rohan requested review of this revision.Jan 30 2023, 9:01 AM

ashoat added inline comments.Jan 30 2023, 12:22 PM

native/chat/settings/thread-settings.react.js
625 ↗	(On Diff #21620)	What's this condition for?
633–637 ↗	(On Diff #21620)	Might be best to tuck the gallery away behind another screen. Something that you'll want to discuss with Ted

Might be best to tuck the gallery away behind another screen. Something that you'll want to discuss with Ted

Personally think two rows of most recent media + a "see more" sort of button would make sense

In D6467#193470, @atul wrote:

Might be best to tuck the gallery away behind another screen. Something that you'll want to discuss with Ted

Personally think two rows of most recent media + a "see more" sort of button would make sense

Agreed, that seems to also be inline with what Ted and I discussed this morning for native. This linear task tracks this, and I'll update it with more info once I hear back about the design confirmation https://linear.app/comm/issue/ENG-2876/display-three-media-initially-then-allow-user-to-see-more-on-native

native/chat/settings/thread-settings.react.js
625 ↗	(On Diff #21620)	I was unsure if we want to display the media gallery for sidebars because we hadn't had the chance to discuss it, so this check can be removed if we'd like to display the media gallery everywhere.

rohan added a child revision: D6482: [keyserver] Attach a threadID to each media (image/video) upload.Jan 31 2023, 10:26 AM

ashoat requested changes to this revision.Jan 31 2023, 11:56 AM

ashoat added inline comments.

native/chat/settings/thread-settings.react.js
625 ↗	(On Diff #21620)	I don't understand what this has to do with sidebars... you seem to be checking if the `parentThreadID` is a specific thread. I don't understand why this check would be something we want to land

This revision now requires changes to proceed.Jan 31 2023, 11:56 AM

Remove unnecessary check in thread settings. Tested to ensure the media gallery appears in the settings still.

Harbormaster completed remote builds in B15929: Diff 21748.Feb 1 2023, 5:50 AM

Passing back to you with question

native/chat/settings/thread-settings.react.js
638 ↗	(On Diff #21748)	Do we not need a footer?

This revision now requires changes to proceed.Feb 1 2023, 10:27 AM

Add a footer, visually it definitely makes sense to have and the padding seems better.

rohan added inline comments.Feb 1 2023, 1:07 PM

native/chat/settings/thread-settings.react.js
638 ↗	(On Diff #21748)	Just updated to include a footer - it definitely looks better and separates the media gallery better having the footer included.