Update route summary tooltip with correct time and correct URL Routing#51
Update route summary tooltip with correct time and correct URL Routing#51wdcsimon wants to merge 3 commits intovalhalla:masterfrom
Conversation
Time is reported wrong in route summary tooltip when hovering over route. Apply same fix as used in Issue valhalla#12
Use # instead of / for correct URL routing
Correct URL routing for directions and isochrones
wdcsimon
changed the title
nilsnolde
left a comment
nilsnolde
left a comment
There was a problem hiding this comment.
Thanks for the fix. Just one thing to clarify.
| queryParams.set('profile', profile) | ||
|
|
||
| let path = '/directions?' | ||
| let path = '#directions?' |
There was a problem hiding this comment.
This seems wrong, can you elaborate here?
There was a problem hiding this comment.
Basically "directions" or "isochrones" are not physical files, but handlers, so they should follow a # rather that a / as this provides correct URL routing. If we use the / then if a user does a browser page refresh, they end up generating a 404-error page, by using the # we avoid this potential issue. See https://maps.whanganui.govt.nz/routing to see this in action.
There was a problem hiding this comment.
Basically "directions" or "isochrones" are not physical files, but handlers
Right, as a web dev noob I figured that's mostly syntactical in real life if using / or #.
What I can see on your demo page, it's not properly resolving permalinks, i.e. page refreshes fail to parse the full URL into a parameterized version of the web app. E.g. this: https://maps.whanganui.govt.nz/routing/#directions?profile=bicycle&wps=175.02682766786324%2C-39.91050190801741%2C175.04880524660373%2C-39.93498734998717, would generate a route from the waypoints on https://valhalla.openstreetmap.de/.
Wouldn't you also have to update the function which parses the URL from a permalink?
There was a problem hiding this comment.
Though I'm still open to adhering to proper standards (if that's what it is), but another point is people already pointing to the app, in all the issues in upstream Valhalla etc. If there's no really good reason to change it, I wouldn't like to include that in the merge, it'd mess up all existing links (I know I can tell Apache to deal with this, but again, needs a good reason for me).
There was a problem hiding this comment.
Good point about the permalink, the # doesn't help with that. Will look at how that can be resolved.
Probably better to stick with the / at the moment.
There was a problem hiding this comment.
I think @chrstnbwnkl convinced me about the #, I always wondered why so many SPAs do that. Anyways, I guess we'll just need to update the URL parsing function to trigger permalinks on #. I'd guess it could be done in a backwards-compatible way so that old / URLs are still resolved properly, e.g. from open Valhalla issues. There's custom code in the app for that, I remember.
If you don't have the time for that @wdcsimon let us know and we'll handle it at some point. Thanks anyway!
2 participants
Time is reported wrong in route summary tooltip when hovering over route. Apply same fix as used in Issue #12
Correct URL Routing for directions and isochrones