-
Notifications
You must be signed in to change notification settings - Fork 232
okta-react: SecureRoute updates state outside of render function now #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mschaeffner for the PR! I was able to run this locally and can verify it passes our tests.
Just a small nit before we get this merged in. 👍
@@ -14,6 +14,32 @@ import React, { Component } from 'react'; | |||
import { Route } from 'react-router'; | |||
import withAuth from './withAuth'; | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Lets keep the newlines after imports to 1
as we've done in other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I removed the unnecessary newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmelberg-okta Could you please merge this into master. Then I can also create a PR for #70 . As that also deals with SecureRoute
I'd like to avoid merge conflicts later on.
when will this be merged into master? We are developing an application that needs the exact=true to be passed down to the Route through SecureRoute #70 . |
@jmelberg-okta Any thoughts when this can be merged? |
Sorry for the delay! Bumping @robertjd for final review. |
Thanks for the bump! I'll take a look at this and your other PRs this week. |
Hi again @mschaeffner , unfortunately this slipped the last week but I've set aside time in our new sprint to look a the open PRs for react. |
Is this fixed yet, if so in what version of okta-react? |
Fixes #138