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

chore(legacy-plugin-chart-map-box): bump supercluster to v8 #30305

Merged
merged 1 commit into from
Sep 26, 2024

Conversation

birkskyum
Copy link
Contributor

@birkskyum birkskyum commented Sep 17, 2024

SUMMARY

Part of:

Modernizing the legacy-plugin-chart-map-box reduces the diff towards MapLibre.

This PR updates Supercluster from v4 to v8, which improves performance and memory use for of this plugin.

TESTING INSTRUCTIONS

  • set the mapbox key in .env
  • docker compose up
  • open a mapbox chart, and it should still show clusters like below image
Screenshot 2024-09-17 at 11 15 56

@rusackas , how does the review process generally work for these plugins? Are there some specific people who cover this part of the code?

@dosubot dosubot bot added change:frontend Requires changing the frontend viz:charts:map Related to Map charts labels Sep 17, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@michael-s-molina michael-s-molina added review:checkpoint Last PR reviewed during the daily review standup and removed review:checkpoint Last PR reviewed during the daily review standup labels Sep 17, 2024
@rusackas
Copy link
Member

/testenv up

@rusackas
Copy link
Member

@birkskyum to test/review these PRs, we typically need to check the plugin. There are a few ways to do this:

  1. starting an ephemeral env here so anyone can test it (needs to be done by a committer)
  2. pulling the change locally and running Superset (better in this case so we can add a mapbox key)
  3. pulling the change locally and running Storybook

I'll start with #1 here before I approve. @mistercrunch was your review a code review, or did you test/validate the upgrade?

@mistercrunch
Copy link
Member

I only reviewed the 3 lines, seemed benign, didn't test locally

@rusackas
Copy link
Member

Tested, works great! Thanks, @birkskyum!

@rusackas rusackas merged commit 0fdcd8b into apache:master Sep 26, 2024
40 of 41 checks passed
@rusackas
Copy link
Member

No idea why ephemeral environments aren't working again. Any ideas, @mistercrunch? Did those move to supersetbot or something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:frontend Requires changing the frontend dependencies:npm plugins size/XS viz:charts:map Related to Map charts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants