Replace routing library and fix browser history infinite loop #1040
Conversation
There was a problem hiding this comment.
I remember distinguishing which routes should be ignored and which should be handled was a tricky issue with Navigo, too. The "_top" thing is unfortunate, but this does solve the bug and move to a library that's not essentially unsupported, so that's good. And there's no way to know whether any alternative libraries would be better and worse for this without trying, which would be laborious.
The one thing I found that affects behavior is the bit about the target="_top" attributes needing to be added to the javascript templates.
| // If we're updating the URL from the directions or explore controllers, we | ||
| // don't want to run setPrefsFromUrl again. Calling `done(false)` cancels it. | ||
| updatingUrl = false; | ||
| return; |
There was a problem hiding this comment.
The comment still says "Calling done(false) cancels it", but now it just returns to cancel.
| @@ -149,6 +149,7 @@ <h2 class="place-card-name">{{ destination.name }}</h2> | |||
| href="#">Directions</a> | |||
| {% endif %} | |||
| <a class="place-card-action place-action-details" | |||
| target="_top" | |||
There was a problem hiding this comment.
Having to add these target="_top" attributes to every link is a drag. Also kind of seems like a recipe for some future trouble, i.e. that we'll eventually end up adding a link and forgetting the attribute.
Is there no other way to make this work? I tried a thing or two to get the other workaround suggested from the issue working (i.e. some pattern to successfully differentiate URLs we want to handle from ones we don't) but didn't have any luck.
There was a problem hiding this comment.
Also, these cards get built both with Django templates (on initial load) and by Javascript (after certain user actions, e.g. filtering, origin change). The latter (src/app/scripts/cac/home/cac-home-templates.js) needs to be kept in sync.
There may be other pairs of templates with the same problem.
Overview
Replaces the routing library
navigowithriot-route, enabling us to fix an issue with URL routing that resulted in an infinite redirect in the browser history for URLs missing some parameters.Notes
The Riot routing library hijacks events for all links that match the base URL (in our case,
/).In order to work-around this, I added
target="_top"to any inter-application links on the home/explore pages, which was suggested as a work-around in the issue: riot/route#25.Testing Instructions
vagrant provision appChecklist
No python lint warningsPython tests passFixes #1015
Fixes #1024