Page MenuHomePhabricator
Differential D6073

[native] style SidebarListModal, SubchannelsModal, ThreadListModal
ClosedPublic
Actions

Authored by inka on Dec 28 2022, 8:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 3:40 AM
Unknown Object (File)
Tue, Nov 12, 1:22 AM
Unknown Object (File)
Sun, Nov 10, 2:49 AM
Unknown Object (File)
Fri, Nov 8, 1:16 AM
Unknown Object (File)
Fri, Nov 8, 12:45 AM
Unknown Object (File)
Mon, Nov 4, 11:23 PM
Unknown Object (File)
Mon, Nov 4, 11:23 PM
Unknown Object (File)
Mon, Nov 4, 11:23 PM
View All Files
Subscribers
ashoat
atul

Details

Reviewers
kamil
tomek
michal
• przemek
ashoat
Commits
rCOMM1560bbf996c7: [native] style SidebarListModal, SubchannelsModal, ThreadListModal
Summary

Linear issues: https://linear.app/comm/issue/ENG-2547/restyle-sidebarlistmodal-and-subchannelslistmodal, https://linear.app/comm/issue/DES-20/designs-for-subchannels-modal
SidebarListModal, SubchannelsModal use ThreadListModal undeneath, and no other component uses ThreadListModal. SidebarListModal and SubchannelsModal are supposed to look consistent, and match the new designs provided in the second linked issue.

Screenshot 2022-12-28 at 17.37.19.png (1×516 px, 108 KB)

Screenshot 2022-12-28 at 17.37.58.png (1×512 px, 100 KB)

Screenshot 2022-12-28 at 17.36.54.png (1×516 px, 65 KB)

Screenshot 2022-12-28 at 17.36.33.png (1×514 px, 82 KB)

Screenshot 2022-12-28 at 17.36.20.png (1×508 px, 62 KB)

Test Plan

Run ios simulator, saw that the two modals look as expected. Chceked that scrolling works poperly.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka created this revision.Dec 28 2022, 8:45 AM
Herald added subscribers: atul, ashoat. · View Herald TranscriptDec 28 2022, 8:45 AM
inka edited the summary of this revision. (Show Details)Dec 28 2022, 8:46 AM
inka attached a referenced file: F307989: Screenshot 2022-12-28 at 17.36.20.png. (Show Details)
inka attached a referenced file: F307990: Screenshot 2022-12-28 at 17.36.33.png. (Show Details)
inka attached a referenced file: F307991: Screenshot 2022-12-28 at 17.36.54.png. (Show Details)
inka attached a referenced file: F307992: Screenshot 2022-12-28 at 17.37.58.png. (Show Details)
inka attached a referenced file: F307993: Screenshot 2022-12-28 at 17.37.19.png. (Show Details)
inka added a parent revision: D6062: [native] Remove background from modal items.
Harbormaster completed remote builds in B14842: Diff 20242.Dec 28 2022, 8:57 AM
inka requested review of this revision.Dec 28 2022, 8:57 AM
tomek accepted this revision.Dec 29 2022, 4:09 AM
tomek added a reviewer: ashoat.

Not sure if this is intentional, but x button seems to be shifted left. For me it would look better if the distance from the button to the right edge of the modal was the same as from the title to the left edge.

native/chat/subchannel-item.react.js
92 ↗(On Diff #20242)
native/chat/thread-list-modal.react.js
102–104 ↗(On Diff #20242)

When a lambda contains only a call to one function that has the same list of parameters, it can be skipped:

const f = () => {
...
};

const g = () => f();
is equivalent to
const g = f;

So in this case it could be simplified to just

const close = React.useCallback(navigation.goBack, [navigation]);

But in that case the memoization isn't needed at all.

110 ↗(On Diff #20242)
inka added a comment.Dec 29 2022, 4:42 AM
In D6073#182624, @tomek wrote:

Not sure if this is intentional, but x button seems to be shifted left. For me it would look better if the distance from the button to the right edge of the modal was the same as from the title to the left edge.

It looks a bit weird to me as well, but it's like that in the designs

image.png (860×806 px, 61 KB)

inka updated this revision to Diff 20331.Dec 29 2022, 4:44 AM

Addresss code review, rebase

tomek added a comment.Dec 29 2022, 4:47 AM
In D6073#182751, @inka wrote:
In D6073#182624, @tomek wrote:

Not sure if this is intentional, but x button seems to be shifted left. For me it would look better if the distance from the button to the right edge of the modal was the same as from the title to the left edge.

It looks a bit weird to me as well, but it's like that in the designs

image.png (860×806 px, 61 KB)

I don't think this was intention of the designer. There is another screen https://www.figma.com/file/WmS4u84vnveHHCcZusn5A4/%E2%9A%A1%EF%B8%8F-Mobile-App-Sidedrawer?node-id=5867%3A35597&t=DDA32ZBjU7mH2PZC-0 where the button is in a different place, so I guess the placement is random.

Harbormaster completed remote builds in B14914: Diff 20331.Dec 29 2022, 4:57 AM
inka added a comment.Dec 29 2022, 8:02 AM

Okay, here are some options then:

Here's how it looks with 'x' having the same right margin as the left margin of the title (margin 2 + 16 of header padding):

image.png (1×568 px, 175 KB)

image.png (1×570 px, 196 KB)

And here when the title is shifted to match the 'x' (margin 16 + 16 of header padding):

image.png (1×592 px, 192 KB)

image.png (1×588 px, 173 KB)

And here when the margins are 8 (margin 8 + 16 of header padding):

image.png (1×588 px, 172 KB)

image.png (1×588 px, 192 KB)

Any of those look good? @tomek @ashoat

tomek added a comment.Dec 29 2022, 8:07 AM

For me the 1st option looks the best as the margin of header is consistent with other elements.

ashoat accepted this revision.Dec 29 2022, 8:16 AM

Agree, option 1 looks great!

This revision is now accepted and ready to land.Dec 29 2022, 8:16 AM
inka updated this revision to Diff 20347.Dec 29 2022, 8:24 AM

Fix margins

Harbormaster completed remote builds in B14929: Diff 20347.Dec 29 2022, 8:35 AM
Closed by commit rCOMM1560bbf996c7: [native] style SidebarListModal, SubchannelsModal, ThreadListModal (authored by • InkaSokolowska). · Explain WhyDec 29 2022, 9:26 AM
This revision was automatically updated to reflect the committed changes.
• InkaSokolowska added a commit: rCOMM1560bbf996c7: [native] style SidebarListModal, SubchannelsModal, ThreadListModal.

Revision Contents
Changeset List

Diff 20378

View Options

native/chat/sidebar-list-modal.react.js

Loading...
View Options

native/chat/subchannel-item.react.js

Loading...
View Options

native/chat/subchannels-list-modal.react.js

Loading...
View Options

native/chat/thread-list-modal.react.js

Loading...
View Options

native/themes/colors.js

Loading...