Page MenuHomePhabricator

[native] update message list header to new design
ClosedPublic

Authored by ginsu on Mar 21 2023, 10:46 PM.
Tags
None
Referenced Files
F3363770: D7134.diff
Mon, Nov 25, 3:20 AM
Unknown Object (File)
Thu, Nov 7, 2:54 PM
Unknown Object (File)
Thu, Nov 7, 10:48 AM
Unknown Object (File)
Thu, Nov 7, 10:48 AM
Unknown Object (File)
Thu, Nov 7, 10:47 AM
Unknown Object (File)
Tue, Nov 5, 10:21 PM
Unknown Object (File)
Mon, Nov 4, 11:15 PM
Unknown Object (File)
Mon, Nov 4, 11:14 PM
Subscribers

Details

Summary

Updated the message list header based on how it looks in the figma design doc:
{F445548}

Android was already doing this, so I just needed to remove some conditional logic causing iOS to look different.


Depends on D7132

Test Plan

Please check out these screenshots to see the changes I made:

Before:

iOS:

Screenshot 2023-03-25 at 11.08.46 PM.png (1×1 px, 786 KB)

Android:

Screenshot 2023-03-25 at 10.39.40 PM.png (1×978 px, 436 KB)

After (pre avatars world):

iOS:

Screenshot 2023-03-25 at 11.09.09 PM.png (1×1 px, 787 KB)

Android:

Screenshot 2023-03-25 at 10.55.13 PM.png (1×978 px, 437 KB)

After (post-avatars world):

iOS:

Screenshot 2023-03-25 at 11.10.05 PM.png (1×1 px, 791 KB)

Android:

Screenshot 2023-03-25 at 10.53.19 PM.png (1×934 px, 324 KB)

Search:

iOS:

Screenshot 2023-03-26 at 2.56.38 AM.png (1×1 px, 761 KB)

Android:

Screenshot 2023-03-26 at 3.01.08 AM.png (1×978 px, 406 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

double checked android rendering

ashoat requested changes to this revision.Mar 22 2023, 12:27 PM
ashoat added inline comments.
native/chat/message-list-header-title.react.js
67 ↗(On Diff #23970)

Why !isSearchEmpty?

68 ↗(On Diff #23970)

You appear to be using the avatar to balance out the icon, but I think that will only appear properly centered if the two components are exactly the same width. Is that the case?

If so, you should avoid having two variables (avatar and fakeIcon), where only one is rendered at a time and they have some overlap in their function. Instead, we should assign to one variable, have an if-else-if structure for setting that variable, and have a comment explaining why we don't need fakeIcon if we have avatar, and that the two are exactly the same width.

If the two aren't the same width, we'll need to think critically about how we make this work and how it's designed. Can you link the Figma designs?

This revision now requires changes to proceed.Mar 22 2023, 12:27 PM
native/chat/message-list-header-title.react.js
67 ↗(On Diff #23970)

This prevents rendering when an avatar in MessageListHeaderTitle when the user is trying to create a new chat thread

Screenshot 2023-03-23 at 11.35.18 AM.png (1×1 px, 756 KB)

68 ↗(On Diff #23970)

Double checked the figma design and we should actually be left aligning everything (what we do for android today). This was a screw up on my end from not being careful and doing something without double-checking. Will update accordingly.

ginsu edited the summary of this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)

update message list header to new desgin

ginsu retitled this revision from [native] render thread avatar in message list header title to [native] update message list header to new design.Mar 26 2023, 12:02 AM
ginsu edited the summary of this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)
ashoat added inline comments.
native/chat/chat.react.js
237 ↗(On Diff #24142)

Can you check with Ted to confirm that we still want to center the header title in the chat creation design?

This revision is now accepted and ready to land.Mar 26 2023, 5:57 AM
native/chat/chat.react.js
237 ↗(On Diff #24142)

Just spoke to Ted IRL and he says to center the header title in chat creation