Skip to content
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

Refactor and update the create/find campaign flow #447

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

wdoug
Copy link
Member

@wdoug wdoug commented Nov 4, 2018

What does this PR do?

  • Refactors the routing code to make it a bit more manageable
  • Renames a bunch of components to no longer have the Render prefix
  • Deletes a little bit of unused code (There is a bunch more that could probably be deleted)
  • Splits logic that used to be in FirebaseChooseCampaign.js into two separate routes that are redirected to after the firebase search returns (I believe this closes Create campaign state flash after create new campaign #441)
    • handleRedirectToExistingCampaign => redirects to new /existing-campaign route
    • handleRenderNearbyCampaigns => currently not handled (may come back to it in the future)
    • handleRenderNewCampaign => redirects to new /create-campaign route
  • Adds a wrapping component for authenticated routes that redirects to an initial login page if the user isn't signed in
  • Makes the new /create-campaign route require authentication using the above component (Closes Handle UI for create campaign when not logged in #440)

@wdoug wdoug had a problem deploying to denver-reimagine-stage-pr-447 November 5, 2018 23:24 Failure
@wdoug wdoug had a problem deploying to denver-reimagine-stage-pr-447 November 5, 2018 23:28 Failure
@wdoug wdoug had a problem deploying to denver-reimagine-stage-pr-447 November 6, 2018 03:16 Failure
@hoop71
Copy link
Contributor

hoop71 commented Nov 6, 2018

  • Added NewCampaign container to handle auth state while selecting a campaign
  • Cleaned up logic and redirect. Moving Redirect functionality into Redux Actions

Things to look for. Does the routing work in different auth states. Does FirebaseInitialSearch or other parts of the store need to be cleared at any point>

@hoop71 hoop71 requested a review from julian009 November 6, 2018 03:18
Copy link
Contributor

@dwhite96 dwhite96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good as far as I can tell.

…on SubmitButton optional to avoid undefined prop error
@wdoug wdoug had a problem deploying to denver-reimagine-stage-pr-447 November 8, 2018 21:15 Failure
@wdoug wdoug had a problem deploying to denver-reimagine-stage-pr-447 November 8, 2018 21:20 Failure
Copy link
Collaborator

@julian009 julian009 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid work on the routing refactor. Everything seems to be working as expected. A couple things we may want to polish off before merging into master/production:

  • Add styles to Login component
  • Maybe: In the case where a user is not logged in but tries to access a protected route, should the AuthenticatedRoutes component first pop off the most recent protected route from the router stack before pushing the Login path on top? This way when a user tries to navigate backward after being redirected to the Login screen by using the back button of the browser, they will go to the route that they were on before attempting to access the protected route instead of navigating back to the protected route and immediately being redirected to Login again.

@wdoug wdoug added this to In Progress in codefordenver/Circular Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Create campaign state flash after create new campaign Handle UI for create campaign when not logged in
4 participants