Realted linear issue: https://linear.app/comm/issue/ENG-2285/add-hamburger-menu-button-to-tabs
Updating some flow types that relate to navigators headers, as I need bottom tab navigators header definitioon for community drawer.
Details
Details
run flow in comm/native/. Cross referencing with ts types, although there are some differences between ts types and react naviagtion docs. I found that sometimes ts types have more fields that
docs specify. In these cases I have omitted these fields.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
BottomTabHeaderProps and DrawerHeaderProps are based on react navigation docs [ 1, 2 ].
StackHeaderLeftButtonProps and HeaderLeftButtonProps are used to type headerLeft.
StackHeaderLeftButtonProps are based on the ts definition [headerLeft definition, , HeaderBackButtonProps used by headerLeft definition].
HeaderLeftButtonProps are used for bottom tab navigator and drawer navigator based on ts definitions [ 1, 2 ], where headerLeft is defined in HeaderOptions [ 1 ].
HeaderCommonOptions are based on react navigation docs for bottom tab, stack and drawer navigators. So are specific header options for each navigator.
Stack Header Options in ts include a field headerBackTestID that is not present in the react navigation docs. I have omitted this filed to not cause errors.
The ts definition for HeaderOptions, that is a part of definition for bottom tab navigator header options, includes a field headerLeftLabelVisible that is not present in the react navigation docs definition of bottom tab navigators header options. I have omitted this filed to not cause errors. Similarly for drawer and stack navigators.
Based on react navigation docs I also assume that Material Bottom / Top Tabs don't have any header options.
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1373 ↗ | (On Diff #19065) | Not sure about this flied. StackHeaderLeftButtonProps is used to type headerLeft as +headerLeft: StackHeaderLeftButtonProps => React$Node, for stack navigator. In react navigation docs headerLeft is defined as "Function which returns a React Element to display on the left side of the header. ". So I refferred to ts types where headerLeft for stack navigator is typed as headerLeft?: (props: HeaderBackButtonProps) => React.ReactNode;, where HeaderBackButtonProps include this field. But on the other hand StackHeaderOptions in ts include a field headerBackTestID that is not present in the react navigation docs. The Stack Header Options are otherwise well defined by the docs, so I decided to omit that filed. Don't know what is the right thing to do here. |
Comment Actions
I can review this one since I have the most context on React Navigation Flow libdefs
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1342–1354 ↗ | (On Diff #19065) | Avoid copy-paste here |
| 1342–1354 ↗ | (On Diff #19065) | This seems like the wrong place to put tab-specific and drawer-specific things |
| 1377 ↗ | (On Diff #19065) | I'm confused, does stack actually have a different HeaderLeftButton component than the other navigators? |
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1342–1347 ↗ | (On Diff #19065) | We probably can skip using explicit exact syntax {|...|} and instead use just {...} because Flow is now exact by default. My guess is that most of the types here were written before the transition and weren't updated since then. @ashoat am I right, or there is a reason behind preferring explicit syntax in libdefs? |
| 1373 ↗ | (On Diff #19065) | I don't think it makes sense to expose any testID prop. Usually things like this are used in test frameworks to assign a special id and select a node effectively based on it. So in this case I guess that this is something react navigation uses in their own tests and probably doesn't want anyone to use - that's why it isn't in the docs. So a solution here is to skip any test related prop. |
| 1406–1408 ↗ | (On Diff #19065) | Is there a reason behind header left and right props being different? Maybe we can use the same parameter? |
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1342–1347 ↗ | (On Diff #19065) | This is a libdef, we should not make assumptions about .flowconfig having exact_by_default=true, and consequently avoid using ambiguous {} syntax. Still have hopes of contributing this back upstream at some point |
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1377 ↗ | (On Diff #19065) | In ts stack header options use the options shared by other navigators, but omit headerLeft, headerTitle, headerRight and redefine them. And headerLeft is actually different, unlike headerTitle that is just redefined to the same type.... |
| 1406–1408 ↗ | (On Diff #19065) | I mean... they just are differently typed in the ts files, but headerRight actually needs to be parametrised, I need to change this. |
Comment Actions
native/chat/chat-header.react.js, native/chat/chat.react.js, native/navigation/header.react.js, native/profile/profile-header.react.js, native/profile/profile.react.js files had to be changed because of extracting from StackHeaderProps props common for the three navigators.
HeaderButtonProps and StackHeaderButtonProps are based on ts types used for headerRight (and a subset of type for headerLeft for Stack) [ 1, 2, 3 ].
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1339 ↗ | (On Diff #19108) |
|
Comment Actions
Super close!! Deferring to you on checking TypeScripts & docs to make sure these types are correct, and also to make sure every single "SECTION 1" is identical :)
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1375–1377 ↗ | (On Diff #19129) | Can we spread HeaderButtonProps here instead of duplicating the params? Also makes sense to move this to below StackHeaderButtonProps since it is similar |
| 1381 ↗ | (On Diff #19129) | Is this type used anywhere other than HeaderTitleInputProps? If not maybe we should get rid of it and just define HeaderTitleInputProps directly. What do you think? |
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1381 ↗ | (On Diff #19129) | It is used in elements_v1.x.x.js in SECTION 2: declare export type StackHeaderTitleProps = $Partial<HeaderTitleInputBase>; |
Comment Actions
Checked if SECTION 1 is the same with a text comparison function from VSCode, so it should be all good.
Going to double check the correctness with ts types and docs.
Please let me know if since HeaderTitleInputBase is used in elements_v1.x.x.js, it's fine to leave it as is.
Comment Actions
Requesting changes to clarify the organization of these files. I can see that we have stack related types in a couple of places. Is it required?
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1339 ↗ | (On Diff #19169) | Why do we have a stack specific type in a file that is about a different type of navigator: the bottom tab? |
| 1398 ↗ | (On Diff #19169) | Is this empty line needed? |
| 1419 ↗ | (On Diff #19169) | Shouldn't this be in stack file? |
Comment Actions
We keep almost all the types in SECTION 1 to make it easier to edit these files. That way, edits can just be blindly copy-pasted across the different files rather than constantly having to ask "Which file does this belong to? Does this type affect any other files?" I'm not sure if the types you called out are used elsewhere @tomek, but this is how we've done things historically and I think it's out-of-scope to address that in this diff
Comment Actions
Fix navigation and progress prop types, move header generic options out of stack navigator section
| native/flow-typed/npm/@react-navigation/bottom-tabs_v6.x.x.js | ||
|---|---|---|
| 1313–1315 ↗ | (On Diff #19201) | I realised I had all header types in the Stack options section, so I moved the shared ones out |
| 1317–1321 ↗ | (On Diff #19201) | Based on ts source code |
| 1324 ↗ | (On Diff #19201) | I realised that stack, bottom tab and drawer navigators have the navigation field in the type for header typed with *NavigationProp, not *NavigationHelpers. I fixed that in places where I pass the parameters. |