Linear issue: https://linear.app/comm/issue/ENG-1881/community-navigation-drawer-on-native
Add a navigator for community drawer.
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Can you be more specific? You seem to have navigator props here, so I'm not sure what you mean. Do you mean that AppNavigationProp is the wrong type? Or maybe you mean that the downstream navigator (TabNavigator) is using AppNavigationProp but is no longer a child of AppNavigator, so the types should be updated?
To be clear, it's important to make sure you are typing things correctly – just because you're not seeing Flow errors doesn't mean that everything is okay.
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 46 ↗ | (On Diff #18860) | Can we avoid using Dimensions here? |
Comment Actions
Yes, I meant that TabNavigator is using the wrong type, but actually in this diff TabNavigator is still under AppNavigator and DrawerNavigator is not yet used anywhere, so I'll make these changes in D5734, where DrawerNavigator is added to the navigation hierarchy
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 46 ↗ | (On Diff #18860) | Hmm, honestly I think it would be better to address now. What's your reasoning for deferring this work? Deferring light mode makes sense since you want designers to take a look, but getting the basic style right I think can be done now |
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 46 ↗ | (On Diff #18860) | I created a separate task for styling so I could put up some diffs and get the review process of the code logic started. Since there is a styling task I figured it'd be better to have all styling discussions in diffs that will be created for that task, so it's easier to navigate if someone needs to refer to these discussions later. But I don't feel strongly about this, that's just what was intuitive to me. |
| 46 ↗ | (On Diff #18860) | I took some time trying to change this and here is what I found: Looking at the react navigation source code for DrawerView I can see that indeed drawerStyle takes only these two values, overlayStyle takes only backgroundColor, and the only other style related option is sceneContainerStyle that refers to how the inner screen is displayed (if I set it to marginRight=10 the chat list view gets a margin, so I definitely cannot use this). So it seems to me like this cannot be done differently than just setting the width in drawerStyle in screenOptions. |
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 46 ↗ | (On Diff #18860) | How does React Navigation handle making sure that the drawer doesn't take the full width of the screen? Separately, it's hard to tell from this diff where CommunityDrawerNavigator is used. In general you should not have a diff that just introduces an unused component... the diff should set that component to be used somewhere. Eg. if this was used as drawerContent then I would suggest checking the default DrawerContent component in React Navigation |
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 46 ↗ | (On Diff #18860) | in React Navigation Drawer component renders the View containing the drawer content (the visible thing that slides from the side) and gives it a style style={[
styles.container,
right ? { right: offset } : { left: offset },
{
transform: [{ translateX: drawerTranslateX }],
opacity: this.drawerOpacity,
zIndex: drawerType === 'back' ? -1 : 0,
},
drawerStyle as any,
]}Where styles.container has a default width: '80%'. drawerStyle comes from props, passed to it from DrawerView like so: <Drawer
// *snip*
drawerStyle={{
backgroundColor: drawerBackgroundColor,
width: this.state.drawerWidth,
}}
// *snip*
/>So the only way to change how much space the visible drawer takes up is to pass drawerStyle.width |
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 46 ↗ | (On Diff #18860) | I don't understand. React Navigation is able to style its default drawer content view to make this work without using Dimensions, correct? I do not understand why you are insisting on doing it using Dimensions. It seems like you should be able to make CommunityDrawerNavigator work exactly the same way as the component it is replacing. |
Comment Actions
At this point we are 3 layers deep in a "Request Changes" / "Re-request Review" cycle, which is very much an anti-pattern. If I am definitely, for sure missing something please let me know... but please don't re-request review unless you're confident that your explanation is simple / concise / thorough enough that I can understand it in quickly, and won't request changes again
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 46 ↗ | (On Diff #18860) | The width of a drawer component is determined based on width style property. So it is possible to provide a value that is either a percent or an absolute value. Also, CommunityDrawerNavigator works exactly as the original component. What isn't possible, for both of these, is to set an exact width of the space that isn't covered by the drawer. So you can't say that a drawer is calc(100% - 32px). You also can't specify the width of the uncovered space directly - it is determined as the remaining space. The only solution that doesn't require changing the library code is to specify the width of the drawer in pixels. We can do that quite easily by using a hook and computing a value by subtracting the space that is left. This is an approach that is widely used and @inka provided some examples of it. One thing to consider is why do we need exactly 32 or 16 px. Why don't we simply use a default 80% or any other value? This solution was suggested 6 days ago https://linear.app/comm/issue/ENG-2343/style-the-navigation-drawer-on-native#comment-832aca08. What I think is concerning is that we're maybe paying too much detail to this problem. A solution using dimensions hook works correctly and seems to be the easiest one. Spending this amount of effort on this doesn't bring any value and may cause our goals to be in danger. |
Comment Actions
Okay, I think I understand now. The simple way to explain this:
- React Navigation's default approach uses width: 80%, which we can't use here because the designs do not call for it
- React Navigation's Drawer component uses a width-based approach, so we can't change that to be flex-based
I'm sorry I can be a difficult reviewer... I usually only have 3 minutes or so per diff 😅
The only solution that doesn't require changing the library code is to specify the width of the drawer in pixels. We can do that quite easily by using a hook and computing a value by subtracting the space that is left. This is an approach that is widely used and @inka provided some examples of it.
I agree here – we definitely should not use Dimensions raw like this, we should instead be getting the most recent Dimensions value (since it can change during execution).
However, this can be handled in a follow-up task. @inka, can you create a follow-up task for this? I'd like something more specific than the generic "styling" task.
One thing to consider is why do we need exactly 32 or 16 px. Why don't we simply use a default 80% or any other value? This solution was suggested 6 days ago https://linear.app/comm/issue/ENG-2343/style-the-navigation-drawer-on-native#comment-832aca08.
Fair point. When I responded there, I wasn't aware that React Navigation only really supported pixel or percent values, so we have to choose between percent and using some sort of Dimensions hook. The designer who implemented that screen has since left the team, so we'll have to make a decision without them. Can you maybe create a new design ticket and ask @davidg about this?
Before landing please:
- Create the ENG task (for yourself) for making this into a hook
- Create the DES task for figuring out if 80% is fine
- Address the inline comment
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 30–32 ↗ | (On Diff #18860) | This should be React.useMemo'd |
Comment Actions
Task for using a hook for dimensions
Task for discussing the width
I will try to get better at explaining what I mean, but it's hard sometimes.
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 38 ↗ | (On Diff #19164) | Are we sure that Flow doesn't allow us to use CommunityDrawerContent directly? Ultimately, it is a function that doesn't take arguments, so it should be possible to use here (but maybe Flow disagrees with that). |
Comment Actions
I will try to get better at explaining what I mean, but it's hard sometimes.
No problem – I also can do better at reading carefully
| native/navigation/community-drawer-navigator.react.js | ||
|---|---|---|
| 38 ↗ | (On Diff #19164) | Flow doesn't allow that. It says that React.ComponentType is incompatible with function type. |