Linear issue: https://linear.app/comm/issue/ENG-1881/community-navigation-drawer-on-native
Add a custom drawer content implementation to be passed to DrawerNavigator, displaying a CommunityDrawerItemCommunity for each community.
Details
Details
- Reviewers
tomek atul ginsu kamil - Commits
- rCOMM437282e001bc: [native] Add CommunityDrawerContent
Added this component to AppsDirectory and check that it allows to navigate to threads, the elements open and close as expected. Checked that when only one community is present it is open by defalut and none is open when there are more
communities. Checked that the app dooesn't crash when there are no communities.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/navigation/community-drawer-content.react.js | ||
|---|---|---|
| 11–12 ↗ | (On Diff #18854) | We can merge these imports |
| 21–22 ↗ | (On Diff #18854) | We should avoid redefining variables - it reduces readability and maintainability. In this case we can either create a new variable to hold communities with the suffix, or do that as a part of the selector. It seems like the first option should be more performant. |
| 25–27 ↗ | (On Diff #18854) | It's better to avoid using numbers with special meaning. When we have no community open, we should rather use null to indicate that. Also, is there a requirement to open the first community by default? Shouldn't we keep all of them closed initially? |
| 32–38 ↗ | (On Diff #18854) | We should use a callback and a version of set state where (state) => new state argument is used. |
| 40 ↗ | (On Diff #18854) | Use callback |
| 56–88 ↗ | (On Diff #18854) | It might be a good idea to move this to a function with descriptive name. |
| 73–76 ↗ | (On Diff #18854) | Do we have a constant that describes all these types? Do we use the same set of types in other places? With current implementation it would be extremely hard to find all the places that have to be updated if we decide to introduce a new thread type. |
Comment Actions
Address review
| native/navigation/community-drawer-content.react.js | ||
|---|---|---|
| 25–27 ↗ | (On Diff #18854) | The requirement is that when there is only one community it should be open by default. I don't think this will open a community when there are more than one... I have two communities and none of them is open by default. |
| native/navigation/community-drawer-content.react.js | ||
|---|---|---|
| 56–61 ↗ | (On Diff #19168) | We can use a simpler notation |
| 71–72 ↗ | (On Diff #19168) | It is a good idea to add type annotations for these to make the code more readable |
| 72 ↗ | (On Diff #19168) | In context of this function the fact that community names have suffixes doesn't matter and we can skip this so that the name is simpler |
| 25–27 ↗ | (On Diff #18854) | Makes sense! |