added isochrone color palettes and opacity control#340
added isochrone color palettes and opacity control#340nilsnolde merged 5 commits intovalhalla:masterfrom
Conversation
|
Preview is ready! 🚀 You can view it here: https://valhalla-app-tests.gis-ops.com/340 |
|
Hii @nilsnolde |
|
@s4chinjha pls don't send reminders before 3-4 days of no review. We're all aware of our projects and get to it when we have the time. This PR was also in draft until a few hours ago, which is communicating that it's not ready yet. |
nilsnolde
left a comment
There was a problem hiding this comment.
few things:
- hardly any tests, you need to add some that are reasonable
- comment style issue(s)
- this PR is breaking current color picking for isochrones, see further below
26453bb to
27fd413
Compare
I am really sorry for that and will keep in mind to not send reminder before 3-4 days. |
I have tried to fix them, just take a look if the changes are good or any improvements are required. |
|
I commented on that before:
force-pushing after a review makes my life a LOT harder as a reviewer. pls re-write the history, so that you have a single commit pre-review and then at least one commit post-review, so that I can see what changed after my initial review. |
|
I realize that I'm being quite pedantic & strict. however, GSoC is all about learning and IMO the first thing to learn is open source etiquette. |
I am really sorry again for repeating the same mistake. But, I would like to explain why I force-pushed in the first place. For ref: I wanted to ask what workflow you would prefer in this situation. If I make several commits addressing review comments, should I keep them as separate commits instead of squashing them (to avoid force-pushing), or is squashing them into one commit acceptable? I’d like to follow the workflow that makes reviewing easiest and learn the correct way to handle this going forward. |
I think that's how it should be everywhere. Having someone like you guiding us on these practices is really valuable, so I appreciate the feedback and the time you take to review. |
|
Will remove the comments. |
@s4chinjha I think my comment should explain that fairly ok. basically, try to be clean: 1 commit per post-review fix (e.g. 4 change requests -> 4 post-review commits with a good commit message). it's not so bad if you break that, that's just for me the most ideal situation. but really, zero force pushes after initial review, never if you can. |
|
IMO open source projects are the best to learn being a software engineer/developer. it should really be part of every comp sci program, e.g. complete 1000 hours of documented contributions in well-established OSS projects. that'd make it eventually so much more tolerable to work for tech companies. |
|
@mustaphaturhan i could not see your suggestion before, the one you added 2 days ago.Most of the comments were for me i forgot to remove them really sorry. |
Yep, they really are one of the best places to learn. If documented contributions became part of our coursework, I think we would learn far more than many lectures ever teach us. |
hm, that's not true though, colors change depending on the no. of contours. I actually had in mind what you're writing here, so that all isochrones have a fixed color, e.g. largest one: max(color), 2nd largest one: max(color) - 1, 3rd largest one: max(color) - 2 etc. while I think that's better UX when changing intervals, the current PR implementation is more future proof, we should only need min & max of the palette. |
nilsnolde
left a comment
There was a problem hiding this comment.
from UX this fine for me now. wdyt @mustaphaturhan ?
|
the current UX looks fine to me as well. I just can't be sure if we would want to show the isochrone settings in the left panel or in the right, settings panel. either way, implementation looks fine. |
|
yeah the isochrone settings are currently a bit hidden, or at least for me it's hard(er) to notice them |
|
thanks @s4chinjha ! |
Thanks for the clarification and for approving the Pr, glad the current approach works for the UX. |
This PR Closes #283 and adds an Isochrone Settings Section to the isochrone summary panel.
🛠️ Fixes Issue
As mentioned in issue #283, the default isochrone colors are way too opaque and are not very accessible (see also #191). There is also no way of changing opacity right now, which makes it harder to read the map underneath when multiple contours overlap.
👨💻 Changes proposed
New file -
src/utils/isochrone-palettes.tsAdded a utility that defines the available color palettes and handles color interpolation across any number of contours.
Color Palette selector
Added an option for the user to choose from a few predefined color scales - Default, Viridis (colorblind-friendly), Plasma, and Blues.
The Default option keeps the existing API-provided colors so nothing changes for users who don't touch the setting.
Opacity slider
Added a slider (0–1) to control fill transparency. Useful when contours overlap or when the map details underneath need to stay readable.
Both controls update the map live - no need to redraw the isochrone after changing palette or opacity.
Palette is a global setting, not per-isochrone, so it will continue to work correctly when multiple isochrones are supported in the future.
Fixed rendering order so smaller contour rings are always visible on top of larger ones instead of being hidden underneath.
Updated tests to include the new store fields so all existing tests continue to pass.
I have used AI in assisting me