Skip to content

fix: disable delete/reset buttons when all waypoints are empty regard…#383

Open
rohhann12 wants to merge 4 commits intovalhalla:masterfrom
rohhann12:master
Open

fix: disable delete/reset buttons when all waypoints are empty regard…#383
rohhann12 wants to merge 4 commits intovalhalla:masterfrom
rohhann12:master

Conversation

@rohhann12
Copy link

…less of order

Closes #382

🛠️ Fixes Issue

Minor ux Bug: Shuffling the waypoints enables the delete and reset waypoint buttons, though no location is added in the waypoints.

Changed the logic of how the disabling those buttons were working

👨‍💻 Changes proposed

instead of comparing waypoints against defaultWaypoints via JSON.stringify (which breaks on reorder), we simply check if every waypoint's userInput is empty.

In both the files-

  • src/components/directions/directions.tsx
    -src/components/directions/waypoints/waypoint-item.tsx
    I changed the logic of disabling the button instead comparing the waypoints against the default , now we simply check if they are empty.

📄 Note to reviewers

📷 Screenshots

1CC38228-E72E-4A99-909B-57CFEB1C823F.mov

@github-actions
Copy link

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

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.

please make sure there's some github PR hygiene in place, like a proper non-overflowing title. and if this is for GSoC, I'd give you the benefit of the doubt, as nothing here suggests AI usage. in future PRs just do note that anyways pls, even the guidelines don't explicitly say so..

other than that, LGTM, thanks!

nilsnolde
nilsnolde previously approved these changes Mar 18, 2026
@rohhann12
Copy link
Author

Thanks for the review! will make sure of the points mentioned.

The failing test is because my disabled condition doesn't account for when extra waypoints have been added.The test adds a 3rd empty waypoint and then tries to reset back to 2 — which is a valid use case, but my condition waypoints.every((wp) => !wp.userInput) disables the button since all inputs are empty.

I'll fix it by updating the condition to:disabled={waypoints.every((wp) => !wp.userInput) && waypoints.length <= 2}
This way the reset button is only disabled when all waypoints are empty and the count is already at the default (2). If the user added extra waypoints, they can still reset even if nothing was typed.

Will push the fix shortly.

@rohhann12
Copy link
Author

mistake- pushed a wrong commit here , will clean it up

@rohhann12
Copy link
Author

fixed it @nilsnolde

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.

BUG: Shuffling the waypoints enables the delete and reset waypoint buttons, though no location is added in the waypoints.

2 participants