-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
200 -redirect user to login if not logged in and tries to access '/project/new' #208
base: develop
Are you sure you want to change the base?
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://websiteone-fe-git-fork-alau2088-200-userredirect.agileventures1.now.sh |
hey thanks for putting this in @ALau2088! 😸 |
src/components/App.js
Outdated
cookies={this.props.cookies} | ||
/>) | ||
}} | ||
<PrivateRoute exact path='/projects/new' component={NewProject} /> |
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.
why do we need a PrivateRoute
here?
we can do something similar to the CreateEventPage
where we save a user's lastLocation
in our redux store, check if a user is logged in, and if not redirect them to the login page.
src/components/NewProject.js
Outdated
|
||
export default function NewProject() { | ||
return ( | ||
<Fragment> |
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.
we already have a way to create new projects, please check out CreateProjectPage
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.
maybe the issue was not clear enough? let me make update to explicitly state this
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.
Got it. Just seeing the CreateProjectPage now. I will update my fork and make the requested changes shortly.
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.
great @ALau2088, thanks for making the requested changes! 😸
just a slight change to the tests is needed see 👇
da54869
to
a480331
Compare
@ALau2088 currently failing the build with this output
can you try updating it similar to this? |
Deployment failed with the following error:
|
@mattwr18 . I made the requested edit and it passed the CircleCI test. |
@mattwr18 @aonomike . Hey guys, it just occurred to me that these changes were made to the CreateProjectPage prior to converting to redux-form. I could go ahead and implement this in the redux-form version of the components but before I do just wanted to check with you guys if this is the correct approach. Thanks in advance. |
there is another option I just find is formsy-semantic-ui-react . https://github.com/zabute/formsy-semantic-ui-react |
hey @ALau2088, sorry I am just replying to this now, I have been real busy the last couple days. We need to talk about it in the next meeting, which will be Tuesday... up to you if you want to wait till then or not, but I think if you do want to go ahead and refactor, we can always open up a ticket later that you or someone else can work on. At the moment, there are merge conflicts, so if you could rebase before you make any changes, it will make your life a bit easier. |
Thanks @ALau2088 I ran this locally and looks great. Maybe we can have a session to add cypress tests? |
@aonomike Sure. Want to have session tomorrow Tuesday 5/7 10am-12pm Pacific Standard Time? |
Fixes #200 |
Any progress on this @ALau2088?? |
Hey @mattwr18 . Thanks for the reminder. I added a setLastLocation test and redirect test and am ready to push it up to the PR but my branch files (CreateProjectPage.js and CreateProjectPage.test.js are behind the upstream files. My initial thought was to merge the upstream and branch but I wanted to see if you had any thoughts on going about this? Also I was curious what is the use of the setLastLocation action? |
you can run |
893e318
to
f297de6
Compare
Thanks @mattwr18 . The changes have been pushed up to the PR. |
Issue addressed
fixes #200
Screenshots (if appropriate):
Testing