Remove panning logic from map boundary calculation #27392
Conversation
| if (antemeridianGapSize > normalGapSize) { | ||
| return L.latLngBounds( | ||
| L.latLng(south, west), // SW | ||
| L.latLng(north, east), // NE | ||
| ); | ||
| } else { | ||
| return L.latLngBounds( | ||
| L.latLng(south, -360 + gap[1]), // SW | ||
| L.latLng(north, gap[0]), // NE | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
as far as i can tell, the only purpose of this conditional, and the computeLargestGap function, was to determine if a map was large enough to warrant panning.
|
Notifying subscribers in CODENOTIFY files for diff 2d8b306...96ff2a2.
|
| ); | ||
| } else { | ||
| return L.latLngBounds( | ||
| L.latLng(south, -360 + gap[1]), // SW |
There was a problem hiding this comment.
this -360 is the giveaway. longitude only goes to +/- 180, so no matter what the biggest gap is, this is going to give us a left boundary wider than the globe
Codecov ReportBase: 64.98% // Head: 64.97% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #27392 +/- ##
==========================================
- Coverage 64.98% 64.97% -0.01%
==========================================
Files 3180 3184 +4
Lines 92471 92664 +193
Branches 11748 11770 +22
==========================================
+ Hits 60088 60208 +120
- Misses 27532 27605 +73
Partials 4851 4851
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
paoliniluis
force-pushed
the
gh-26213-custom-region-maps
branch
from
a592f4d
to
f16a04f
Compare
iethree
force-pushed
the
gh-26213-custom-region-maps
branch
from
f16a04f
to
0f886dd
Compare
|
Looks like I need to update the visual tests to account for loading time for maps:
|
iethree
added
the
backport
|
No failed tests 🎉 |
* remove panning logic from map boundary calculation * add map viz percy tests * wait for map load in visual tests
Resolves #26213
Story Time
Chapter 1
Once upon a time, some maps in metabase would repeat themselves, to make the left-right panning experience nicer: e.g. it would be easier to center a global map on Japan, even though most global maps center on Europe. This functionality was removed in #22427 because it looked a little funny and users reported it as a bug. In order to make maps appear correctly after repeating was removed, we added this code to center maps:
metabase/frontend/src/metabase/visualizations/components/LeafletChoropleth.jsx
Line 103 in ccac9d9
And all was well. (spoiler: it was not all well).
Chapter 2
Later on, we discovered an issue where users "couldn't load" new maps #23450 (spoiler: in fact, maps would load just fine, their previews just wouldn't appear for some reason)
It appeared that the panning code actually was pushing the maps out of the viewport, and removing the panning code fixed everything! (spoiler: it did not fix everything)
Chapter 3
And it came to pass that again, we have a reported issue that certain maps seem to be squished to the side and cut off 🤔
But some maps are totally fine:
What if we just stick the panning code back in?
Hooray! we've saved the world, but it cost us California, the UK and the Pacific Islands. 😢
Alright? what's really going on here?
Well, it turns out that in the code that sets map boundaries, for certain large maps, it sets ridiculously large boundaries (wait for it...) to allow you to pan side to side.
Changes
Deleted the conditional and calculations that set map boundaries wide enough to allow side-to-side map panning.
And all the maps are happy now.
Testing Steps
I used all of these test maps to make sure they appeared correctly, in the map loader (in admin settings), and in the query builder, and in dashboards.
Gonna poke around a bit and see if there's a way to unit test this. 🕵️♀️It appears that JSDOM doesn't really support svg rendering, so any meaningful unit tests are impossible with our current test setup.
https://stackoverflow.com/questions/54382414/fixing-react-leaflet-testing-error-cannot-read-property-layeradd-of-null/54384719#54384719