Skip to content

feat(legacy-plugin-chart-map-box): Add fixed initial viewport settings option #26736

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

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

jason0598647
Copy link

@jason0598647 jason0598647 commented Jan 22, 2024

SUMMARY

It's currently not possible to fix the initial map box view. The current settings are not used when setting up the initial view.
The initial is auto calculated from bounds, and in some cases doesn't seem to place all the points on the screen.
This PR adds a drop down to enable a "fixed" initial view option which will use the user entered settings as the initial view instead of the calculation from bounds.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

TESTING INSTRUCTIONS

By default, the MapBox chart will still auto calculate the initial view from the bounds.
To test the fixed option, set the Initial Viewport settings dropdown to fixed and update the default lat/long/zoom

ADDITIONAL INFORMATION

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.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 22, 2024
@jason0598647
Copy link
Author

jason0598647 commented Jan 22, 2024

The last commit fixes an issue where the plots do not load until the map is dragged.
The first time the component loads after login works fine, but after that, the plots do not load until dragged.
Putting the clusters in state fixes the issue. Also converted it to a functional component.

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 69.62%. Comparing base (6443001) to head (9433499).
Report is 2420 commits behind head on master.

Files with missing lines Patch % Lines
...plugins/legacy-plugin-chart-map-box/src/MapBox.jsx 0.00% 28 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #26736      +/-   ##
==========================================
+ Coverage   69.07%   69.62%   +0.54%     
==========================================
  Files        1930     1909      -21     
  Lines       75279    74770     -509     
  Branches     8429     8350      -79     
==========================================
+ Hits        51999    52058      +59     
+ Misses      21133    20661     -472     
+ Partials     2147     2051      -96     
Flag Coverage Δ
javascript 57.37% <0.00%> (+0.89%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@justinpark justinpark left a comment

Choose a reason for hiding this comment

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

@jseparovic, thank you for your contribution to the mapbox options. I've left some comments on the PR that might be of assistance. Your efforts are greatly appreciated!

@jason0598647
Copy link
Author

jason0598647 commented Feb 4, 2024

@justinpark Thanks for the review! I updated the PR to use useMemo as per your suggestion, but unfortunately it broke the fix for when the plots do not load until the map is dragged. The latest commit I pushed shows the working code where i added // ++ comments for the added lines and // -- for the removed lines so you can see what the code looked like with useMemo.
Any ideas on how to fix this bug with useMemo? Essentially the issue is that when the map loaded for the first time after logging in, it works ok, but then if you go back to that dashboard, or refresh the page, the plots do not load until you drag the map. The useEffect+useState fixes this, but the useMemo how I implemented it does not. Did I miss something?

This reverts commit c1a28d1.
@github-actions github-actions bot added doc Namespace | Anything related to documentation plugins labels Mar 19, 2024
@jason0598647 jason0598647 force-pushed the mapbox_add_fixed_initial_option branch from a8ad9b8 to 9433499 Compare March 19, 2024 02:53
@github-actions github-actions bot removed the doc Namespace | Anything related to documentation label Mar 19, 2024
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

I think this looks OK to me, but might like a second pair of eyes from @michael-s-molina who is uniquely aware of the risks of upgrading/migrating form data schemas from version to version.

@rusackas
Copy link
Member

@jseparovic I think this just needs the [precommit hooks] run - one of the linters (prettier, specifically) is not happy with the changelog file.

@rusackas
Copy link
Member

Marking this as draft until it can be made mergeable via pre-commit fixes and a rebase. Please feel free to mark it as ready for review when it's lookin' good, otherwise it may be closed in time if it doesn't get any love.

@rusackas rusackas marked this pull request as draft March 21, 2025 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot set default latitude/longitude in MapBox chart
4 participants