improvement(example): Move Auth0Provider plumbing out of example.js a… #175

binh-dam-ibigroup · 2020-06-15T23:04:48Z

This pull request addresses this comment: #167 (comment) and moves the plumbing to set up Auth0Provider to the ConnectedRouter component.

…nd to ConnectedRouter.

codecov-commenter · 2020-06-15T23:08:11Z

Codecov Report

Merging #175 into add-trip-rq-history will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@                  Coverage Diff                   @@
##           add-trip-rq-history    #175      +/-   ##
======================================================
- Coverage                 9.94%   9.91%   -0.04%     
======================================================
  Files                      117     117              
  Lines                     4131    4144      +13     
  Branches                  1095    1101       +6     
======================================================
  Hits                       411     411              
- Misses                    3224    3237      +13     
  Partials                   496     496

Impacted Files	Coverage Δ
lib/components/app/app-nav.js	`0.00% <0.00%> (ø)`
lib/components/app/responsive-webapp.js	`0.00% <0.00%> (ø)`
lib/index.js	`0.00% <0.00%> (ø)`
lib/util/auth.js	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f39e30...b7a04bc. Read the comment docs.

landonreed

This approach feels better to me. See comments about moving utils to actions and renaming some things.

example.js

landonreed · 2020-06-16T13:37:13Z

lib/util/auth.js

+ * @param dispatch The dispatch method to carry out the actions.
 */
-export function getAuth0Callbacks (reduxStore) {
+export function getAuth0Callbacks (dispatch) {


These callbacks are effectively actions, right? I feel like there are some gymnastics happening to try to make everything a util function instead of an action. I don't really like the idea of passing dispatch from the component as a param here. I think this is a spot where things could be simplified by making these actions and connecting them in mapDispatchToProps. Conventions exist for a reason: we do things in a common predictable way so that the app can be navigated easily by the rest of the team. Also, I don't think we need to combine them all into a getAuth0Callbacks function.

Sample code legacy...

landonreed · 2020-06-16T13:38:04Z

lib/components/app/responsive-webapp.js

+    const { dispatch, persistence, routerConfig } = this.props
+    // Initialize Auth0Provider in this component so it is available everywhere.
+    const auth0Config = getAuth0Config(persistence)
+    const auth0Callbacks = getAuth0Callbacks(dispatch)


As mentioned in another comment, these callbacks should be connected to the component as actions.

Good point. Very good point.

landonreed · 2020-06-16T13:39:42Z

lib/components/app/responsive-webapp.js

+            domain={auth0Config.domain}
+            clientId={auth0Config.clientId}
+            redirectUri={URL_ROOT}
+            {...auth0Callbacks}


Please avoid spreading these props. There's only three of them and our goal should be readable, maintainable code.

This and turning the auth0 callbacks into actions are addressed in 0cf1737.

lib/components/app/app-nav.js

landonreed · 2020-06-16T13:44:04Z

lib/components/app/app-nav.js

+            <div
+              className={`icon-${branding}`}
+              // This style is applied here because it is only intended for
+              // desktop view.


Not sure this comment is necessary since this file is only intended for Desktop use as far as I can tell.

At some point we'll have to figure out combining this and the mobile navigation bar.

lib/components/app/app-nav.js

lib/components/app/responsive-webapp.js

landonreed

Looks great! Just a couple of minor comments that need addressing. Also, it looks like the build is failing.

landonreed · 2020-06-19T18:05:56Z

lib/components/app/desktop-nav.js

+ * The desktop navigation bar, featuring a sign-in button/menu.
+ * TODO: merge with the mobile navigation bar.
+ */
+const DesktopNav = ({ otpConfig, title }) => {


I'm still a little confused as to why there is a title prop. Shouldn't this be taking the title from otpConfig?

I don't know! OpenTripPlanner was hardcoded as title before, and trimet-mod-otp implementations don't use a title at all, so I was trying to accommodate both.

OK, so previously example.js just had a very barebones navbar and was just serving as an example for how to piece together otp-react-redux components within a web frame. However, now that we have otp-ui, I think it's OK for example.js to change from that original purpose somewhat. That's all just backstory to give you a little more context. It looks like the goal here is to make this DesktopNav available to trimet-mod-otp, right? So, in that case, we basically only want one or the other between branding and title, right? So, keep the title prop, but do not have a defaultValue. If titleis missing, usebrandingif it's available. If both are missing just useOpenTripPlanner`. But please (!) add some comments explaining this stuff.

Using title from otpConfig in bb9f9e8.

landonreed · 2020-06-19T18:07:37Z

lib/actions/auth.js

+/**
+ * This function is called by the Auth0Provider component, with the described parameter(s),
+ * after the user signs in.
+ * @param {Error} err


copy pasta param?

Pasta is sticky...

landonreed · 2020-06-19T18:09:36Z

lib/actions/auth.js

+      // Here, we save the URL hash prior to login (contains a combination of itinerary search, stop/trip view, etc.),
+      // so that the AfterLoginScreen can redirect back there when logged-in user info is fetched.
+      // (For routing, it is easier to deal with the path without the hash sign.)
+      const urlHashWithoutHash = (appState.urlHash.split('#')[1] || '/')


Is it possible for the split to yield an array of length = 1? I'm wondering if it's risky to try to access the second element (for fear of array out of index error). Is a better string operation to use here appState.urlHash.replace('#', '')?

I agree it'd be good to avoid array index out of bounds exceptions, although, I think a brute force replacement of the hash character isn't nuanced enough.

Using substring instead of array.split in bb9f9e8.

evansiroky

What Landon said. 😆

…nts.

landonreed

This looks great, @binh-dam-ibigroup. Thanks for all the work on this.

landonreed · 2020-07-09T19:24:43Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

improvement(example): Move Auth0Provider plumbing out of example.js a…

b7a04bc

…nd to ConnectedRouter.

binh-dam-ibigroup requested review from evansiroky and landonreed June 15, 2020 23:05

binh-dam-ibigroup assigned landonreed and evansiroky Jun 15, 2020

binh-dam-ibigroup mentioned this pull request Jun 15, 2020

Log trip requests #167

Merged

landonreed requested changes Jun 16, 2020

View reviewed changes

landonreed assigned binh-dam-ibigroup and unassigned landonreed Jun 16, 2020

refactor(actions/auth): Extract auth callbacks into actions.

0cf1737

binh-dam-ibigroup requested a review from landonreed June 17, 2020 21:07

binh-dam-ibigroup assigned landonreed and unassigned binh-dam-ibigroup Jun 17, 2020

landonreed approved these changes Jun 19, 2020

View reviewed changes

landonreed assigned binh-dam-ibigroup and unassigned landonreed Jun 19, 2020

evansiroky approved these changes Jun 22, 2020

View reviewed changes

evansiroky removed their assignment Jun 22, 2020

refactor(DesktopNav): Use otpConfig.title for title. Address PR comme…

bb9f9e8

…nts.

binh-dam-ibigroup requested review from evansiroky and landonreed June 22, 2020 19:57

binh-dam-ibigroup assigned landonreed and evansiroky and unassigned binh-dam-ibigroup Jun 22, 2020

landonreed approved these changes Jun 22, 2020

View reviewed changes

landonreed removed their assignment Jun 22, 2020

binh-dam-ibigroup merged commit ae3c44d into add-trip-rq-history Jun 22, 2020

binh-dam-ibigroup deleted the auth0provider-inside-router branch June 22, 2020 20:28

landonreed added the released label Jul 9, 2020

improvement(example): Move Auth0Provider plumbing out of example.js a… #175

improvement(example): Move Auth0Provider plumbing out of example.js a… #175

Uh oh!

Conversation

binh-dam-ibigroup commented Jun 15, 2020

Uh oh!

codecov-commenter commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

landonreed left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

landonreed left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

landonreed Jun 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evansiroky left a comment

Choose a reason for hiding this comment

Uh oh!

landonreed left a comment

Choose a reason for hiding this comment

Uh oh!

landonreed commented Jul 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jun 15, 2020 •

edited

Loading

landonreed left a comment •

edited

Loading

landonreed left a comment •

edited

Loading

landonreed Jun 19, 2020 •

edited

Loading