Page MenuHomePhabricator
Differential D6467

[native] Render the media gallery in the thread settings on native
ClosedPublic
Actions

Authored by rohan on Jan 30 2023, 8:46 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 21, 7:20 PM
Unknown Object (File)
Thu, Nov 21, 7:20 PM
Unknown Object (File)
Thu, Nov 21, 6:03 PM
Unknown Object (File)
Thu, Nov 21, 12:51 PM
Unknown Object (File)
Fri, Nov 8, 3:11 AM
Unknown Object (File)
Fri, Nov 8, 3:11 AM
Unknown Object (File)
Fri, Nov 8, 3:11 AM
Unknown Object (File)
Fri, Nov 8, 12:29 AM
View All Files
Subscribers
ashoat

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
No Lint Coverage
Unit
No Test Coverage

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

atul added a comment.Jan 30 2023, 12:24 PM

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

rohan added a comment.Jan 31 2023, 9:09 AM
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
rohan updated this revision to Diff 21748.Feb 1 2023, 5:35 AM
rohan edited the test plan for this revision. (Show Details)

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
ashoat requested changes to this revision.Feb 1 2023, 10:27 AM

Passing back to you with question

native/chat/settings/thread-settings.react.js
638

Do we not need a footer?

This revision now requires changes to proceed.Feb 1 2023, 10:27 AM
rohan updated this revision to Diff 21785.Feb 1 2023, 1:05 PM

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

Just updated to include a footer - it definitely looks better and separates the media gallery better having the footer included.

ashoat accepted this revision.Feb 1 2023, 1:10 PM
This revision is now accepted and ready to land.Feb 1 2023, 1:10 PM
Harbormaster completed remote builds in B15953: Diff 21785.Feb 1 2023, 1:23 PM
rohan edited the summary of this revision. (Show Details)Feb 2 2023, 6:59 AM
rohan updated this revision to Diff 22417.Feb 13 2023, 6:33 AM

Rebase

Harbormaster completed remote builds in B16380: Diff 22417.Feb 13 2023, 6:49 AM
rohan updated this revision to Diff 23107.Feb 26 2023, 11:31 AM

Rebase

This revision was landed with ongoing or failed builds.Feb 26 2023, 11:46 AM
Closed by commit rCOMM6e2f92634d55: [native] Render the media gallery in the thread settings on native (authored by rohan). · Explain Why
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMM6e2f92634d55: [native] Render the media gallery in the thread settings on native.
Harbormaster completed remote builds in B16916: Diff 23107.Feb 26 2023, 11:46 AM

Revision Contents
Changeset List

PathSize
native/
chat/
settings/
33 lines

Diff 21748

View Options

native/chat/settings/thread-settings.react.js

Loading...