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.

Sign up for GitHub

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

Update route summary tooltip with correct time and correct URL Routing #51

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+22 −7
Open

Update route summary tooltip with correct time and correct URL Routing #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d3d2f4
Update route summary tooltip with correct time
wdcsimon Jan 16, 2023
6e3d2f1
Correct URL routing for directions and isochrones
wdcsimon Jan 16, 2023
4ef54a0
Merge pull request #1 from wdcsimon/patch-1
wdcsimon Jan 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions src/Map/Map.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import PropTypes from 'prop-types'
import axios from 'axios'

import * as R from 'ramda'
import { intervalToDuration } from 'date-fns'
import ExtraMarkers from './extraMarkers'
import { Button, Label, Icon, Popup } from 'semantic-ui-react'
import { CopyToClipboard } from 'react-copy-to-clipboard'
Expand Down Expand Up @@ -526,11 +527,25 @@ class Map extends React.Component {
}

formatDuration = (durationInSeconds) => {
const date = new Date(durationInSeconds * 1000)
const days = date.getDate() - 1 > 0 ? date.getDate() - 1 + 'd ' : ''
const hours = date.getHours() > 0 ? date.getHours() + 'h ' : ''
const minutes = date.getMinutes() > 0 ? date.getMinutes() + 'min' : ''
return days + hours + minutes
const duration = intervalToDuration({
start: 0,
end: durationInSeconds * 1000,
})

let durationStr = ''
if (duration.days > 0) {
durationStr += duration.days + 'd '
}
if (duration.hours > 0) {
durationStr += duration.hours + 'h '
}
if (duration.minutes > 0) {
durationStr += duration.minutes + 'min '
}
if (duration.seconds > 0) {
durationStr += duration.seconds + 'sec'
}
return durationStr
}

getRouteToolTip = (summary, provider) => {
Expand Down
4 changes: 2 additions & 2 deletions src/actions/commonActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export const updatePermalink = () => (dispatch, getState) => {
const queryParams = new URLSearchParams()
queryParams.set('profile', profile)

let path = '/directions?'
let path = '#directions?'
Copy link
Owner

Choose a reason for hiding this comment

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

This seems wrong, can you elaborate here?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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).

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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!

if (activeTab === 0) {
const wps = []
for (const wp of waypoints) {
Expand All @@ -77,7 +77,7 @@ export const updatePermalink = () => (dispatch, getState) => {
queryParams.set('wps', wps.toString())
}
} else {
path = '/isochrones?'
path = '#isochrones?'

let center
for (const result of geocodeResults) {
Expand Down