Skip to content
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

fix initial bubble marker bug #604

Merged
merged 3 commits into from
Nov 7, 2023
Merged

fix initial bubble marker bug #604

merged 3 commits into from
Nov 7, 2023

Conversation

moontrip
Copy link
Contributor

@moontrip moontrip commented Nov 7, 2023

Hi @dmfalke (and @bobular):

As commented at #597 (comment) about a bug concerning the bubble marker, I tried to find the reason and fix the bug. After delving into the issue, I realized that it was because marker data is prefetched via useQuery while the legendData via useLegendData hook is still being undefined. This leads the bubble markers to be not shown: more specifically all the marker data have 0 diameters & no colors due to undefined bubbleValueToColorMapper & bubbleValueToDiameterMapper functions at legendData. After sleeping on this, I finally found two possible solutions as follows:

a) Based on the pre-existing comment by Dave, "FIXME Don't make dependent on legend data", I tried to move legendData and the relevant parts to use legendData, i.e., finalMarkerData, out of useMarkerData, i.e. move them to the parent where useMarkerData is called, and then adjusted the parent accordingly. Based on my tests, this worked fine.

b) That being said, an idea came to my mind as an alternative solution after a while. So, basically this can also be considered as an issue of not calling useQuery again (refetch) when legendData is ready (not undefined), thus I changed the conditions of "enabled" option used in the useQuery by additionally checking if legendData is undefined. Certainly, this is much simpler solution than a) and I confirmed that this also worked out.

Due to simplicity, I made PR for b). What do you think @dmfalke?

@moontrip moontrip requested a review from dmfalke November 7, 2023 03:31
Copy link
Member

@dmfalke dmfalke left a comment

Choose a reason for hiding this comment

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

Good detective work! I think this is a fine solution. Thanks!

I know this is in draft mode, but I think it's ready to merge.

@moontrip
Copy link
Contributor Author

moontrip commented Nov 7, 2023

@dmfalke 🕵️
Thank you for your review and confirmation! 👍 I will merge this to main after updating this branch with the latest main

@moontrip moontrip marked this pull request as ready for review November 7, 2023 15:02
@moontrip moontrip merged commit 4fa8fc9 into main Nov 7, 2023
1 check passed
@moontrip moontrip deleted the fix-initial-bubble-marker branch November 7, 2023 15:34
@bobular
Copy link
Member

bobular commented Nov 7, 2023

Any reason not to close #597, @moontrip?

I'm looking at #603 now but that can be a separate issue!

Edit: thanks for fixing!!

@moontrip
Copy link
Contributor Author

moontrip commented Nov 7, 2023

@bobular No problem! Honestly, I am not quite sure if my fix is the same with your issue, #597, so can you please test it with your working branch?

@bobular
Copy link
Member

bobular commented Nov 7, 2023

I guess this fixes the first bug/screenshot in #597 and then I started rambling on about a second bug which I have described in more detail in #603. I'll edit #597 and close it.

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.

3 participants