-
Notifications
You must be signed in to change notification settings - Fork 902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[historical-view] Changes colors on the map when timeslider is used #4080
Conversation
if (isHistoryFeatureEnabled && isLoaded && co2ColorScale) { | ||
// TODO: This will only change RENDERED zones, so if you change the time in Europe and zoom out, go to US, it will not be updated! | ||
// TODO: Consider using isdragging or similar to update this when new zones are rendered | ||
const features = ref.current.queryRenderedFeatures(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should just initially start by querying all features here. I did so in my prototype with quite good performance.
Then we can optimize it through dragging if we'd like later on, I'm worried this will cause some odd experiences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, in your prototype (on mk/map-color-feature-states
) we also change only color of the zones that are rendered as far as I can tell :)
Kapture.2022-05-05.at.15.39.58.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, you're right. Maybe there's a way to circumvent this, but your approach seems to be as intended.
I just added IsDragging to the dependency array, and it actually works great since it will both trigger on dragStart and dragEnd. It also triggers when zooming out but it doesn't trigger too much.
I also tried viewport but that fires way too much.
I suggest we just add the isDragging for now? Maybe that's enough to fix everything :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for testing and good point, that definitely solves some of the problems - let's roll with it! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)
I am having some issues testing this. Can you describe how to get the featureToggle working?
Also, I think we'd also need to update the tooltip for this feature to really work in itself. We can handle it in a seperate PR since this is behind a featureToggle if you'd prefer, but I think it ties into this quite well.
Thanks for the review! You can test with And good point with the tooltips, I've created a new issue for it and will work on it in a separate PR. EDIT: Ah wait, there is a bug where the mockserver can't handle if query parameters start with
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just approving, but I think we could add isDragging to the useEffect and see if that doesn't just fix everything.
Turns out fixing the tooltip was very simple, so I went ahead and did it in this PR too :) |
Description
part of #3938
This will make the colors on the map change for all countries where we have loaded historical data, whenever the time-slider is dragged.
For now this will only happen when a user has already clicked a zone, but that will change when we starting using
v5
state :)Preview