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

Improve large route ui (show step number) #1856

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

twitwi
Copy link
Contributor

@twitwi twitwi commented Jan 19, 2020

This PR proposes two fine tunings to the web UI. I'm not sure whether this part should be covered by additional unit tests, and/or should be split.

A first commit uses plain CSS to put a number in front of each step in the routing (to be able to see the correspondence with the markers on the map).

A second commit adds a checkbox that, when checked, prevents the automatic-rezoom features that takes place when the user adds an intermediate destination (this feature is very annoying when fine tuning very long tracks).

@easbar
Copy link
Member

easbar commented Jan 19, 2020

A first commit uses plain CSS to put a number in front of each step in the routing (to be able to see the correspondence with the markers on the map).

This is very useful and fixes #1359, thanks!

image

@easbar
Copy link
Member

easbar commented Jan 19, 2020

A second commit adds a checkbox that, when checked, prevents the automatic-rezoom features that takes place when the user adds an intermediate destination (this feature is very annoying when fine tuning very long tracks).

Yes, when you use start and endpoints that are far apart from each other, then zoom in to find some location, and then add an intermediate location the map zooms out. This can be annoying if you want to add multiple locations, I guess? On the other hand not sure if this justifies adding an extra checkbox, because the maps demo is supposed to be very minimal. Could we just always disable the zoom out when adding intermediate points?

@twitwi
Copy link
Contributor Author

twitwi commented Jan 20, 2020

Indeed, it probably does not deserve a checkbox (to be honest even a global javascript boolean would be enough for me). As a default, it is probably better to not zoom when adding a point.

I'll try to update the PR to not zoom (but only when the user adds points), the initial zoom feature (when opening a link) is very useful.
I'm not sure when I can do it though.

@easbar
Copy link
Member

easbar commented Jan 23, 2020

If you'd like to have the first commit merged already now before you find the time to remove the check box feel free to create a separate PR (usually creating small PRs that focus on a single issue is the easiest way to merge a PR quickly). And you also need to sign the license agreement (not sure if you did already: https://github.com/graphhopper/graphhopper/blob/master/CONTRIBUTING.md)

@twitwi twitwi changed the title Improve large route ui Improve large route ui (show step number) Jan 26, 2020
@twitwi
Copy link
Contributor Author

twitwi commented Jan 26, 2020

I limited the PR to the first commit (the one that fixes #1359) and send the signed CLA by email.

@karussell karussell merged commit 9518488 into graphhopper:master Jan 27, 2020
@karussell
Copy link
Member

Thanks!

@karussell karussell added this to the 1.0 milestone Jan 27, 2020
@karussell karussell added the web label Jan 27, 2020
@karussell
Copy link
Member

This is now live on GraphHopper Maps :)

It seems that it starts counting differently on chrome (with 2 instead 1). Do you know what the problem could be?

@twitwi
Copy link
Contributor Author

twitwi commented Jan 27, 2020

It's my mistake, I used an unsuported feature... https://caniuse.com/#search=counter-set

The fix is to

  1. remove this rule
#locationpoints {
    counter-set: pointcounter -1;
}
  1. add :nth-child(n+2) to get
#locationpoints>div:nth-child(n+2) {
    counter-increment: pointcounter;
}

@twitwi
Copy link
Contributor Author

twitwi commented Jan 27, 2020

I'll try to do a PR (later) unless it's faster to do it directly for you.

@twitwi
Copy link
Contributor Author

twitwi commented Jan 28, 2020

PR sent, #1874

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants