search

gis-ops / valhalla-app

This is the demo web app running on https://valhalla.openstreetmap.de
https://valhalla.openstreetmap.de
MIT License
163 stars 90 forks source link

Use default value when a user enters a value out of valid range #70

Closed theWiseAman closed 1 year ago

theWiseAman commented 1 year ago

Pull Request Description

theWiseAman commented 1 year ago

@nilsnolde Will this do? This way we won't need to display any warnings when a user inputs any large or too-small number which is not within the min and max range.

nilsnolde commented 1 year ago

This is helpful too. And should be fine for now.

Actually it was about the warnings array in the Valhalla response. It's best to have client- & server-side checks. But more generally, Valhalla can respond with warnings which doesn't have anything to do with wrong input, e.g. a second pass for the routing or whatever, the user has no influence on that. So what we'd like is that there's a toast or so that pops up and presents the user with all warnings that were generated by Valhalla. See https://valhalla.github.io/valhalla/api/turn-by-turn/api-reference/#outputs-of-a-route.

It does depend a bit on an open PR I have since ages: https://github.com/valhalla/valhalla/pull/3833. So, that should be done first, otherwise it'll be hard(er) for you to test. All warnings which are currently generated can't be triggered by the web app.

nilsnolde commented 1 year ago

All those open issues were more for us. And we have the full context since we're also maintaining the routing project. I really need to go through these and add more context!

theWiseAman commented 1 year ago

@nilsnolde I think then you could merge this PR. I could check this issue again once the API is fully complete I suppose? Or what else do you suggest?

theWiseAman commented 1 year ago

@nilsnolde I have committed my changes in a different branch. Are you sure you are in the correct branch?

theWiseAman commented 1 year ago

@nilsnolde I have checked it on my remote machine. It's working fine. Should I merge it to master branch? I hadn't merged it in master branch earlier to avoid any conflicts.

nilsnolde commented 1 year ago

Yes, you're right, sorry, must've done smth wrong earlier. Though I'm not going through the ordeal of pulling from all your remotes, I use the patch files from Github: https://patch-diff.githubusercontent.com/raw/gis-ops/valhalla-app/pull/70.patch. It's a quick & easy way to review in IDE IMO. Probably I was on the wrong patch, currently going through all PRs.