Follow up to comments on PR1559 #1756

marcaaron · 2021-03-12T23:02:52Z

Details

cc @tgolen I'm not really sure why I didn't put these in an actions file. Maybe subconsciously was not sure where exactly they even belonged... I don't mind that action files are the only things that should set Onyx data. But still think it's often not clear how to group stuff.

Fixed Issues

Addresses Slack Comment here and @tgolen post merge review on #1559

Tests

Test various aspects of site/app navigation and verify things are working on after the changes

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron · 2021-03-15T18:19:43Z

Just a heads up I'm going to add a few more changes here based on @tgolen's comments. Small improvements and things that we didn't catch in the first pass of #1559

marcaaron · 2021-03-16T17:19:51Z

Sorry thought I had taken this out of draft but I must have forgot.

Updated!

src/ROUTES.js

tgolen · 2021-03-16T19:18:20Z

src/ROUTES.js

+    HOME: '',
+    SETTINGS: 'settings',
+    SETTINGS_PROFILE: 'settings/profile',
+    SETTINGS_PREFERENCES: 'settings/preferences',


NAB: Random thought; these could be nested to make them a little more organized.

Only problem with that is that ROUTES.SETTINGS is already 'settings' so what convention should we use then? Perhaps something like:

SETTINGS: { ROOT: 'settings', },

I'm kind of partial to the underscore convention.

Cool, works for me!

tgolen · 2021-03-16T19:20:09Z

src/libs/Navigation/AppNavigator/createCustomModalStackNavigator.js

            key: PropTypes.string,
        })),
    }).isRequired,

-    // The current view object that has a render method to call
+    // Object containing descriptors for each route with the route keys as its properties


I love these verbose comments!

tgolen · 2021-03-16T19:22:42Z

src/libs/Navigation/AppNavigator/createCustomModalStackNavigator.js

+     * a screen, otherwise any child navigators won't be connected to the navigation tree properly.
+     *
+     * @returns {Object}
+     */
    getCurrentViewDescriptor() {


I remember I wanted to comment about this in the original PR, and I never did, and then when I remembered about it, I couldn't find the spot. So, sorry this is a late comment!

My thought was that it looks like this could be a functional component and just have this logic as part of the function before the return. I'm fine with this being a NAB, but I just didn't think there was any benefit to making this a class component and wanted to get your thoughts on it.

Sure, this is an easy enough change to make and enforces the preference to use function components.

tgolen · 2021-03-16T19:25:29Z

src/libs/Navigation/linkTo.js

-        throw new Error(`The path must start with '/' (${path}).`);
-    }
-
+    const normalizedPath = !path.startsWith('/') ? `/${path}` : path;


I could have sworn we had a utility for adding slashes to the beginning of things, but for the life of me, I can't find it. I can't remember where it was used either... Does anything come to your mind? If not, then ignore this comment.

There is one used in config.js I think, but it adds them to the end not the the front

https://github.com/Expensify/Expensify.cash/blob/9b80316418e057de767ffd7249355d151ea9478c/src/CONFIG.js#L6-L16

Ah yes! That's the one. It might be nice to add these to Str some day.

tgolen · 2021-03-16T19:27:00Z

src/libs/Navigation/linkingConfig.js

@@ -7,79 +9,79 @@ export default {
                initialRouteName: 'Report',
                screens: {
                    // Report route
-                    Report: 'r/:reportID',
+                    Report: `${ROUTES.REPORT}/:reportID`,


Does the /:reportID work with react-navigation? I was trying to find this in their docs and couldn't really find anything. This is what lead me to ask you about the second param for navigate() the other day.

My concern with this is that there is route information here that is not contained within ROUTES.js so it's not apparent when /:reportID should be used and when it shouldn't. It looks to me like the only time that ROUTE.REPORT is referenced is when checking that path has r in the array (I always see it with includes()) so I wonder if there is a better way to do this to still maintain the functionality that is needed.

One solution that comes to mind is having:

// ROUTES.js { REPORT: 'r', REPORT_WITH_ID: `r/:reportID`, }

which would allow all information about the route to be contained in that file. Thoughts?

To answer the first question.... yes! The /:reportID does work with react-navigation there's some more information here:

https://reactnavigation.org/docs/configuring-links/#passing-params

As for the second comment, you raise a good point and I think it does make sense to document these paths as constants with the parameters we expect to use with them. We might not run into too many cases where we are using ROUTES.REPORT_WITH_ID, but it seems like a good way to document how the routes will be used for now.

Cool, thanks for linking that!

tgolen · 2021-03-16T20:42:36Z

src/components/ScreenWrapper.js

@@ -9,8 +10,11 @@ const propTypes = {
    // Array of additional styles to add
    style: PropTypes.arrayOf(PropTypes.object),

-    // Returns a function as a child to pass insets to
-    children: PropTypes.func.isRequired,
+    // Returns a function as a child to pass insets to or a node to render without insets


I love how this solution turned out. It's much better than the dual components that I suggested.

marcaaron · 2021-03-17T02:17:55Z

Thanks for the comments @tgolen ! Updated.

tgolen

looks awesome!

tgolen · 2021-03-17T02:22:49Z

@Beamanator do you want to go through this and take a quick look? Once you approve it, you can go ahead and merge it.

Beamanator · 2021-03-17T14:13:25Z

@tgolen yeah i'd love to check it out this morning! i'll merge when i'm done :D

Beamanator · 2021-03-17T14:45:54Z

Nice updates @marcaaron ! Especially love the change to ScreenWrapper, enabling lots of yummy refactoring :D

add App.js actions file back

959f393

marcaaron self-assigned this Mar 12, 2021

marcaaron requested a review from tgolen March 12, 2021 23:02

tgolen previously approved these changes Mar 12, 2021

View reviewed changes

tgolen requested a review from Beamanator March 12, 2021 23:38

Beamanator previously approved these changes Mar 15, 2021

View reviewed changes

Fix up ROUTES

14e9040

marcaaron dismissed stale reviews from Beamanator and tgolen via 14e9040 March 15, 2021 18:17

marcaaron changed the title ~~Set reportID and currentURL via actions files~~ Follow up to comments on PR1559 Mar 15, 2021

marcaaron added 5 commits March 15, 2021 08:20

fix conflicts

8c7070a

fix copy/pasta comment

10225b8

Use a normalized path in linkTo

ed09c13

re-add leading slash to Linking.getInitialURL() handling

50f4c94

fix parameter passed to /profile

ef73c14

marcaaron marked this pull request as ready for review March 16, 2021 17:19

marcaaron requested a review from a team as a code owner March 16, 2021 17:19

botify requested review from tgolen and removed request for a team March 16, 2021 17:19

tgolen requested changes Mar 16, 2021

View reviewed changes

Make requested changes

3c54273

marcaaron requested review from tgolen and Beamanator March 17, 2021 02:18

tgolen approved these changes Mar 17, 2021

View reviewed changes

Beamanator approved these changes Mar 17, 2021

View reviewed changes

Beamanator merged commit c30cad5 into master Mar 17, 2021

Beamanator deleted the marcaaron-useActionsForOnyx branch March 17, 2021 14:46

github-actions bot locked and limited conversation to collaborators Mar 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up to comments on PR1559 #1756

Follow up to comments on PR1559 #1756

marcaaron commented Mar 12, 2021 •

edited

Loading

marcaaron commented Mar 15, 2021

marcaaron commented Mar 16, 2021

tgolen Mar 16, 2021

marcaaron Mar 17, 2021

tgolen Mar 17, 2021

tgolen Mar 16, 2021

tgolen Mar 16, 2021

marcaaron Mar 17, 2021

tgolen Mar 17, 2021

tgolen Mar 16, 2021

marcaaron Mar 17, 2021

marcaaron Mar 17, 2021

tgolen Mar 17, 2021

tgolen Mar 16, 2021

tgolen Mar 16, 2021

marcaaron Mar 17, 2021

tgolen Mar 17, 2021

tgolen Mar 16, 2021

marcaaron commented Mar 17, 2021

tgolen left a comment

tgolen commented Mar 17, 2021

Beamanator commented Mar 17, 2021

Beamanator commented Mar 17, 2021

Follow up to comments on PR1559 #1756

Follow up to comments on PR1559 #1756

Conversation

marcaaron commented Mar 12, 2021 • edited Loading

Details

Fixed Issues

Tests

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron commented Mar 15, 2021

marcaaron commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Mar 17, 2021

tgolen left a comment

Choose a reason for hiding this comment

tgolen commented Mar 17, 2021

Beamanator commented Mar 17, 2021

Beamanator commented Mar 17, 2021

marcaaron commented Mar 12, 2021 •

edited

Loading