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: facet fails on geoshape #9292

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ChiaLingWeng
Copy link
Contributor

This will close #3729.

Through this fix can solve current problem, I'm not sure if this is where the issue lies.
Let me know if there's any suggestion.

@ChiaLingWeng ChiaLingWeng requested a review from a team as a code owner March 24, 2024 07:42
@joelostblom
Copy link
Contributor

Exciting! Thank you @ChiaLingWeng ! I added an example showing how this works with the code from #3729 (even if that case is more suitable to be used with repeat I think it still illustrates that geo faceting now works).

Let's wait for someone else to have a chance to review the code.

@domoritz
Copy link
Member

Thanks for the pull request. The fix looks a bit brittle to me as it assumes that the data in the component has a particular order. I don't think I want to merge this without a deeper analysis of the issue and why the fix is correct. Could you look a bit deeper into it?

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. The fix looks a bit brittle to me as it assumes that the data in the component has a particular order. I don't think I want to merge this without a deeper analysis of the issue and why the fix is correct. Could you look a bit deeper into it?

@domoritz
Copy link
Member

#3729 has some analysis.

Also, note how existing examples are changing in this pull request which definitely is a red flag.

@domoritz
Copy link
Member

@ChiaLingWeng can you wrap up this pull request?

@ChiaLingWeng
Copy link
Contributor Author

Hi @domoritz, I'll try to close this, but feel free if you want to take it or provide any suggestion!

@domoritz
Copy link
Member

domoritz commented Sep 5, 2024

Thanks for working on wrapping this up. Can you try to make the tests run on your fork?

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.

facet fails on geoshape
3 participants