Add React Navigation Bottom Tabs (rebased)

[adriantoddross]

• Add reactNavigation, and reactNavigationBottomTabs files into boilerplate template
• Set up app with bottom tab navigation by default
• Remove addNavigation command and its tests

Rebase of #28.

[adriantoddross]

Still need to:

• Fix type errors in bottomTabs template
• Set up confirm prompt for bottomTabs

[adriantoddross]

This is ready for review! Looking forward to your feedback.

[adriantoddross]

I'm seeing test errors after setting up a new app with bottom tabs now:

console.error
    Warning: An update to ForwardRef inside a test was not wrapped in act(...).

    When testing, code that causes React state updates should be wrapped into act(...):

    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */

    This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act
....

ReferenceError: You are trying to access a property or method of the Jest environment after it has been torn down. From src/__tests__/App.test.tsx.

      at Timeout.now [as _onTimeout] (node_modules/react-native/jest/setup.js:48:55)

And an error from the test in the App.test.tsx file directly:

`loadSingleFontAsync` expected resource of type `Asset` from expo-asset on native

      10 | // tests can be wrapped with appropriate providers, mocks can be supplied, etc
      11 | export default function render(element: ReactElement): RenderAPI {
    > 12 |   return TestingLibraryRender(
         |                              ^
      13 |     <NavigationContainer>{element}</NavigationContainer>,
      14 |   );
      15 | }

      at node_modules/expo-font/src/FontLoader.ts:46:11

[adriantoddross]

Got the Jest errors resolved.

[stevehanson]

@adriantoddross I'm taking a look at this now! Is it ok if I push up some changes directly to this branch?

[stevehanson]

This is a great start! Since my time is limited to only reviewing on Fridays, I went ahead and implemented some suggested changes in a branch based off of yours. With the complexity of the feature and the number of small things to point out, it was more efficient this way than to have back-and-forth.

See the diff here along with a description of the changes in the commit message. Happy to circle back on any of the changes from there. If you want to use my commit, you can pull it in with git cherry-pick da2bfc9, or feel free to ping me about any thoughts or questions!

[stevehanson]

Thanks again for working on this! Leaving some more feedback since I didn't have time for a full review on Friday. I hope this is enough to go on if you're working on this this week.

[stevehanson]

templates/boilerplate already includes this file, and we don't want to overwrite it when installing React Navigation. What changes were needed here? Is it possible to instead use the CODEGEN:BELT:PROVIDERS injection point? I believe Diego's open Push Notifications PR, #34, might use this is you're looking for an example.

[stevehanson]

Since I think it will simplify things considerably, I also am in favor of removing addNavigation as a top-level CLI command and only supporting installing it in new apps (i.e. as part of createApp). For existing apps, it seems like we would have to make a lot of assumptions for how they're structured to be able to install something as foundational as navigation.

[adriantoddross]

I agree with removing addNavigation as a top level CLI. I'll need to come back and take look at the CODEGEN:BELT:PROVIDERS injection bit here.

[stevehanson]

I think I lean toward installing React Navigation with bottom tabs for all new apps and not having it be configurable in Belt for now. This seems simpler to build into Belt, and users can always remove the bottom tabs if desired, or we could add support for making bottom tabs optional in Belt down the road.

If we install React Navigation (plus bottom tabs) as part of createApp, then the navigation code can just be part of templates/boilerplate, and we wouldn't need to do any injection.

[adriantoddross]

This is a great start! Since my time is limited to only reviewing on Fridays, I went ahead and implemented some suggested changes in a branch based off of yours. With the complexity of the feature and the number of small things to point out, it was more efficient this way than to have back-and-forth.

See the diff here along with a description of the changes in the commit message. Happy to circle back on any of the changes from there. If you want to use my commit, you can pull it in with git cherry-pick da2bfc9, or feel free to ping me about any thoughts or questions!

Nice! I'm on Space Station this week and will take a look!

[adriantoddross]

This is ready for another look. It feels a bit easier to digest and review now.

Seems like most of the work that was left on this was removing addNavigation and updating the dependencies in the boilerplate template needed for bottom tab navigation. Is that right? @stevehanson

I've pulled in your work and made those changes. I'm not sure how our tests should change or what we should add.

[stevehanson]

Great work @adriantoddross! I'm going to go ahead and merge!