Skip to content

Fix elevation profile not updating when route changes#372

Open
antburnett wants to merge 4 commits intovalhalla:masterfrom
antburnett:fix/auto-update-elevation-profile
Open

Fix elevation profile not updating when route changes#372
antburnett wants to merge 4 commits intovalhalla:masterfrom
antburnett:fix/auto-update-elevation-profile

Conversation

@antburnett
Copy link

Summary

  • The height graph data was only fetched when the HeightGraph drawer was expanded/collapsed via onExpand
  • Added a useEffect that automatically fetches height data whenever directionResults changes
  • The existing JSON.stringify comparison prevents duplicate /height API calls

Test plan

  • Set two waypoints and get a route with elevation profile
  • Drag a waypoint to change the route — elevation profile updates automatically without needing to close/reopen the drawer
  • No duplicate /height API calls when route hasn't changed
  • Existing tests pass (8 pre-existing failures unrelated to this change)

The height graph data was only fetched when the drawer was
expanded/collapsed. Added a useEffect to automatically fetch
height data whenever direction results change.
Copy link
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.

Most of these changes have nothing to do with the PR (I think), and I'm not thrilled of merging them upstream unless there's good reasons (fixing some bug, adding some functionality). You can create your own docker image downstream.

Copy link
Member

Choose a reason for hiding this comment

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

Oops! I really don't like the checked in .env. Should be a .env.template..

Copy link
Author

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member

Choose a reason for hiding this comment

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

Can you keep this yourself?

Copy link
Author

Choose a reason for hiding this comment

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

Yes - there was overreach going on with the PR.

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.

2 participants