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

Enable/Disable Single-page app routing #419

Merged
merged 11 commits into from
Nov 13, 2018

Conversation

mikemurray
Copy link
Member

Resolves #417
Impact: minor
Type: feature

Issue

Description of the issue this PR is solving, why it's happening, and how to reproduce it. This may differ from the original ticket as you now have more information at your disposal.

Solution

  • Add an .env var to enable SPA routing
  • Extend next-routes to allow for full page loads
  • Update the link component to allow for full page loads
  • Update PDP to force it into single-page app mode for variant selection
  • Update RoutingStore to for it into single-page app mode when updating the search query (used for the grid filtering)

Breaking changes

ENABLE_SPA_LINKS=true should be placed in your .env file

Testing

With ENABLE_SPA_LINKS=true

  1. Click through the app and make sure everything works
  2. Check the network panel and verify that the page isn't reloaded. (shouldn't see a request for an HTML page)

With ENABLE_SPA_LINKS=false

  1. Click through the app and make sure everything works
  2. Check the network panel and verify that a request for the full page is made
  3. Ensure the PDP variant selection does not force a full page load
  4. Ensure that the grid filter controls do not force a full page load

@impactmass
Copy link
Contributor

@mikemurray thinking about the new ENABLE_SPA_LINKS env; should it have a default value true? since that's the previous SPA behaviour. That way if that ENV is not set, it has the default behaviour

@mikemurray
Copy link
Member Author

@impactmass then maybe DISABLE_SPA_LINKS=true would be better. that way if that doesn't exist, then the default behavior will continue.

@mikemurray
Copy link
Member Author

@impactmass I ended up leaving the .env var the same name and ensuring that the SPA routing is enabled by default.

I also fixed the prop-type warning you were seeing.

Now on to fixing the broken tests.

@mikemurray mikemurray changed the title [WIP] Enable/Disable Single-page app routing Enable/Disable Single-page app routing Nov 12, 2018
@mikemurray
Copy link
Member Author

@impactmass ready for review

Copy link
Contributor

@impactmass impactmass left a comment

Choose a reason for hiding this comment

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

Tested and the full pages loads as described when enabled.

I have a note about the env setup. It feels like a global project concern, but wanted to point it out

next.config.js Outdated Show resolved Hide resolved
@mikemurray
Copy link
Member Author

@impactmass updated the logic and added a comment. Ready for review again.

Copy link
Contributor

@impactmass impactmass left a comment

Choose a reason for hiding this comment

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

Approving.

It would be good to revisit the env setup later.

@impactmass impactmass merged commit 340ec12 into develop Nov 13, 2018
@mikemurray mikemurray deleted the feat-417-mikemurray-spa-links branch November 13, 2018 23:04
This was referenced Jan 15, 2019
@spencern spencern mentioned this pull request Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants