Page MenuHomePhabricator

[native] render thread avatars in community drawer
ClosedPublic

Authored by ginsu on Mar 21 2023, 10:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 28, 8:30 AM
Unknown Object (File)
Mar 8 2024, 1:37 AM
Unknown Object (File)
Mar 8 2024, 1:37 AM
Unknown Object (File)
Mar 7 2024, 9:01 PM
Unknown Object (File)
Mar 7 2024, 5:54 AM
Unknown Object (File)
Mar 7 2024, 5:54 AM
Unknown Object (File)
Mar 7 2024, 5:54 AM
Unknown Object (File)
Mar 3 2024, 12:46 PM

Details

Summary

rendered thread avatar in community drawer. One thing to note is that I did notice that the font size for the thread name was getting decreased the deeper it goes into the community tree. I asked Ted last week if we should do the same with Avatars and he said no. He also said that it would be nice to eventually have the font size for the thread name be the same regardless of level in the community tree. I figured that making the font size all the same might require more discussion/is outside the scope of this diff


Depends on D7135

Test Plan

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

Staff user view

Screenshot 2023-03-24 at 3.04.22 PM.png (1×1 px, 830 KB)

non staff user view:

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

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat requested changes to this revision.Mar 22 2023, 12:42 PM
ashoat added a subscriber: inka.

You should definitely have included @inka on this review – she is by far the expert on this code

native/navigation/community-drawer-content.react.js
66–78 ↗(On Diff #23967)

I don't understand why you're reusing this variable. We should always avoid let if possible (though prefer let to using non-readonly types)

86 ↗(On Diff #23967)

This is a really bad choice on where to inject the style change you want. Looking at createRecursiveDrawerItemsData, it appears that the primary purpose is to share the "depth-selection" logic between native and web. There's no reason that you need to apply your changes so far upstream, forcing yourself to handle things as a batch and with a .map call, as opposed to simply directly updating CommunityDrawerItem.

Separately, while investigating this I identified that we are using a nested FlatList-inside-FlatList approach here, which is bad for scroll behavior. @inka, can you create a follow-up task to address this, and link it here? (@ginsu, please make sure the task is created before landing this diff.)

This revision now requires changes to proceed.Mar 22 2023, 12:42 PM

Please make sure @inka creates the task I requested before you land this

native/navigation/community-drawer-item.react.js
85–98 ↗(On Diff #24144)

You could extracted this code into a component and reuse it in other places (eg. D7134)

This revision is now accepted and ready to land.Mar 26 2023, 6:00 AM
native/navigation/community-drawer-content.react.js
86 ↗(On Diff #23967)
native/navigation/community-drawer-content.react.js
86 ↗(On Diff #23967)

Before landing, can you please sync with @inka to make sure she doesn't have a task already? Given her limited engagement here, it's not clear to me if she is going to follow-up. My goal here was to make sure she engages by blocking you landing this diff on her engagement, NOT just to make sure the task was created. Would be great to see a response from her here, or in the Linear task

native/navigation/community-drawer-content.react.js
86 ↗(On Diff #23967)

Okay for sure, I will ping her right now