Skip to content

feat: add elevation support to export#405

Open
a-y-a-n-das wants to merge 3 commits intovalhalla:masterfrom
a-y-a-n-das:fix-elevation-in-export
Open

feat: add elevation support to export#405
a-y-a-n-das wants to merge 3 commits intovalhalla:masterfrom
a-y-a-n-das:fix-elevation-in-export

Conversation

@a-y-a-n-das
Copy link
Copy Markdown
Contributor

🛠️ Fixes Issue

feat: Add option to include elevation for download

Closes #211

👨‍💻 Changes proposed

  • added a prop in buildDirectionRequest to include elevations.
  • updated useDirectionQuery to accept a prop for the same and return directions with elevations.
  • added a checkbox for elevation in the dropdown.
  • With checked, it makes an additional call with params: elevation_interval: 30 before downloading.

📄 Note to reviewers

AI: used for generating tests.

📷 Screenshots

image

@github-actions
Copy link
Copy Markdown

Preview is ready! 🚀 You can view it here: https://valhalla-app-tests.gis-ops.com/405

Copy link
Copy Markdown
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

A bunch of things are not good tbh:

  • the UI is not up to my standards, looks & feels really odd
image
  • this PR breaks the routing logic: if the page is freshly reloaded and we add 2 waypoints in the map it won't auto-request the route, which it needs to do obviously
  • doing another /route request is way too heavy! I know this feature will hardly be ever used by people, so in practice it doesn't matter much. it's just a good principle to not waste any byte & CPU cycle. use /height instead, that's pretty much for free.

@a-y-a-n-das
Copy link
Copy Markdown
Contributor Author

Yes, sirrr

  • A checkbox without any border looks odd.
  • I tried reloading the preview, and it's fetching the routes.
  • I actually thought of using something other than /route (like the height graph data).

I will try to use that and touch up the UI.

@a-y-a-n-das a-y-a-n-das force-pushed the fix-elevation-in-export branch from b8771b4 to 8b192e0 Compare April 3, 2026 06:43
@a-y-a-n-das
Copy link
Copy Markdown
Contributor Author

a-y-a-n-das commented Apr 3, 2026

image

Now using /height
Implemented using fetch due to this #413

@a-y-a-n-das a-y-a-n-das requested a review from nilsnolde April 3, 2026 06:52
@a-y-a-n-das
Copy link
Copy Markdown
Contributor Author

a-y-a-n-das commented Apr 6, 2026

Please let me know if any further changes are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to include elevation for download

2 participants