Page MenuHomePhabricator

[native] Add drawer item implementations
ClosedPublic

Authored by inka on Nov 24 2022, 7:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 15, 9:01 PM
Unknown Object (File)
Sun, Dec 15, 9:01 PM
Unknown Object (File)
Sun, Dec 15, 9:01 PM
Unknown Object (File)
Sun, Dec 15, 9:01 PM
Unknown Object (File)
Sun, Dec 15, 9:01 PM
Unknown Object (File)
Sun, Dec 15, 9:01 PM
Unknown Object (File)
Sun, Dec 15, 9:01 PM
Unknown Object (File)
Sun, Dec 15, 9:00 PM
Subscribers

Details

Summary

Linear issue: https://linear.app/comm/issue/ENG-1881/community-navigation-drawer-on-native
Add drawer item implementations. There are two types of drawer items - [community] and [a thread in a community]. They behave almost the same, but there is a difference when their expand buttons are toggled: only one community can be open, so opening a community sometimes has to trigger closing another. On the other hand multiple level 1 and level 2 threads can be open at once.
The information about which community is open is thus held by a parent component, and a community is passed an expanded flag and a setExpanded function allowing it to change its parents state.
On the other hand each thread keeps the information about being opened or not in their own state. Why?: They could be passed that information from a parent component, just like community, but then each community (and level 1 chat) would have to keep an array of these states for all its children. The renderItem method would then have to be dependent on this array, so if any child changed its 'open' state, the array would change, and in result all children would rerender.
This is why I created CommunityDrawerItemCommunity and CommunityDrawerItemChat components - to handle opening and closing elements according to the specification and avoid performance issues.

The CommunityDrawerItemChat is not in it's own file, because then I was receiving warnings (with stack traces) about a require cycle between files, since CommunityDrawerItemChat renders CommunityDrawerItem, and CommunityDrawerItem
has children that are CommunityDrawerItemChats. I'm not sure if I should put it in a separate file despite that. It seems like a bad idea, but having two components in one file like this seems unsanitary.

Test Plan

Tested with subsequent diffs.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka held this revision as a draft.
native/navigation/community-drawer-item-cummuninty.react.js
24–40 ↗(On Diff #18820)

Styles will be revisited during styling
ENG-2343

inka published this revision for review.Nov 24 2022, 8:40 AM
native/themes/colors.js
158–161

You need to add colors for both light mode and dark mode. Not sure why this isn't producing Flow errors

Changes due to update on D5718 and add lightmode colours

native/themes/colors.js
86–87 ↗(On Diff #18853)

These values will be revisited during styling ENG-2343

(took a very quick look before something else came up... figured it would be worth leaving comments but not requesting changes)

but having two components in one file like this seems unsanitary

I think it's totally fine when they're so closely related

native/navigation/community-drawer-item-cummuninty.react.js
1 ↗(On Diff #18853)

native/navigation/community-drawer-item-cummuninty.react.js

Let's fix typo in filename

19 ↗(On Diff #18853)

Should we be explicit about the props instead of "spreading"?

(I think @ashoat had thoughts on this at some point but I don't remember)

25–39 ↗(On Diff #18853)

Most of the styles here overlap. My personal preference would be to have some sort of communityBase style and additionally include some sort of communityExpanded style if props.expanded

That being said, this is probably fine for now

native/navigation/community-drawer-item.react.js
17 ↗(On Diff #18853)

Should we move this type to community-drawer-item-community.react.js where it's being used?

Does having two components in one file cause any issues with React HMR?

native/navigation/community-drawer-item-cummuninty.react.js
19 ↗(On Diff #18853)

Spread is great – it's the exact right thing to do here, where we've typed the props as CommunityDrawerItem's props

Does having two components in one file cause any issues with React HMR?

When I change either of the components they get re-rendered as expected. Don't really know what other issues this could cause. I searched on web if someone has had any problems because of this, but noting came up.

native/navigation/community-drawer-item.react.js
17 ↗(On Diff #18853)

Hmm, it's used in this file as well, I don't understand why we would move it.
Since CommunityDrawerItemCommunity is a sort of a wrapper for CommunityDrawerItem it seems logical to me that it would kind of "inherit" the props type (?). I don't know, let me know what you think

tomek requested changes to this revision.Nov 30 2022, 9:32 AM
tomek added inline comments.
native/navigation/community-drawer-item.react.js
19 ↗(On Diff #18853)

I guess this is a function that accepts thread id - maybe we should include this in the signature?

52–54 ↗(On Diff #18853)

This name is misleading. When an item is already expanded, it won't expand it again but instead will close it. So it toggles a state.

72–74 ↗(On Diff #18853)

Do we have to wrap it with View?

109–111 ↗(On Diff #18853)

We have to use callback here.

Also, it is safer to use prev state instead of basing on the value stored in component.

This revision now requires changes to proceed.Nov 30 2022, 9:32 AM

Looks good! Just a couple of minor comments inline.

native/navigation/community-drawer-item-cummunity.react.js
16 ↗(On Diff #19167)

I think we should prefer array over objects, especially when StyleSheet is used

28–40 ↗(On Diff #19167)

Is there a reason to not use StyleSheet?

native/navigation/community-drawer-item.react.js
116 ↗(On Diff #19167)

We can go one step further and use toggle in both callback name and prop name.

This revision is now accepted and ready to land.Dec 6 2022, 4:07 AM
native/navigation/community-drawer-item-cummunity.react.js
28–40 ↗(On Diff #19167)

If I understand correctly, we need to use useStyles to be able to use the colours defined in colors.js.
Indeed in every style defined using StyleSheet.create we have hardcoded colours as far as I can see.
And in every place I checked, if we use colours defined in colors.js we use useStyles.
And if I change this code to use StyleSheet, then the background colour for an expanded community does not appear.

native/navigation/community-drawer-item.react.js
116 ↗(On Diff #19167)

Of course, sorry