Skip to content

Miscellaneous geo fixes - #3856

Merged
etpinard merged 15 commits into
masterfrom
misc-geo-fixes
May 22, 2019
Merged

Miscellaneous geo fixes#3856
etpinard merged 15 commits into
masterfrom
misc-geo-fixes

Conversation

@etpinard

@etpinardetpinard commented May 10, 2019

Copy link
Copy Markdown
Contributor

Three geo fixes.

The fix in 68bd47b is the most interesting where I spent most of the last two days trying to bring back my sane-topojson package back to life (a few things have changed in the Natural Earth dataset and the topojson utility since 2016 😝 ). I settled for a hacky but simple patch for now ... and until sane-topojson is back on solid grounds (cc etpinard/sane-topojson#12)

@plotly/plotly_js reviewing this PR commit-by-commit should be straight-forward enough, but please let me know if you'd like a few codepen examples.

etpinard added 3 commits May 9, 2019 11:23
@etpinard
fix#3831- reset viewInitial stash on *geo.scope* updates
4abc139
@etpinard
resolves#1754- add "no-need-to-fetch-topojson" logic
b04102b
@etpinard
fix#3779- filter out lat<0 features in USA-states location search
68bd47b
This is important as the Natural Earth files
nclude state/provinces from USA, Canada, Australia and Brazil
which have some overlay in their two-letter ids. For example,
'WA' is used for both Washington state and Western Australia.
As subunits from USA and Canada never conflict, filtering out features
south of the equator suffices to fix#3779
A better fix would have us add a "governing unit" properties in subunit features
in the `sane-topojson` package to avoid conflicts.
archmoj
archmoj reviewed May 13, 2019
View reviewed changes
Comment threadsrc/lib/geo_location_utils.js Outdated
@archmoj

archmoj commented May 13, 2019

Copy link
Copy Markdown
Contributor

@etpinard Thanks for the fixes.
💃

@etpinard

etpinard commented May 13, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review @archmoj

I'll leave this PR open for now. I'll try to finish up etpinard/sane-topojson#12 and make the fix in 68bd47b less hacky before 1.48.0 is ready.

@etpinardetpinard mentioned this pull request May 16, 2019
Merged
@etpinardetpinard added the feature something new label May 17, 2019
@etpinard

etpinard commented May 17, 2019

Copy link
Copy Markdown
ContributorAuthor

@archmoj I found time to ✅ etpinard/sane-topojson#12

This PR is reviewable again.

@etpinardetpinard added this to the v1.48.0 milestone May 17, 2019
@archmoj

archmoj commented May 22, 2019

Copy link
Copy Markdown
Contributor

@etpinard It would nice if sane-topojson could generate dists on multiple lines? Then it may be easier to compare the changes e.g. on the GitHub.

@etpinard

etpinard commented May 22, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

It would nice if sane-topojson could generate dists on multiple lines? Then it may be easier to compare the changes e.g. on the GitHub.

It could be nice. Feel free to open a new https://github.com/etpinard/sane-topojson/issues

But comparing MB-large json files isn't my thing, I think comparing diffs like in etpinard/sane-topojson@8624ec4 is much more valuable.

@archmoj

archmoj commented May 22, 2019

Copy link
Copy Markdown
Contributor

Looking good.
Also thanks for sane-topojson upgrades.
💃

@etpinard

etpinard commented May 22, 2019

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review and thanks for spotting the changes over Crimea:

Peek 2019-05-22 15-51

Peek 2019-05-22 15-52

I'll make sure to include a note about that in the changelog.

@etpinard
etpinard merged commit 93af5d6 into masterMay 22, 2019
@etpinard
etpinard deleted the misc-geo-fixes branch May 22, 2019 19:53
@etpinardetpinard mentioned this pull request Jun 12, 2019
Merged
etpinard added a commit that referenced this pull request Jul 15, 2019
@etpinard
baseline geo_lakes-and-rivers using pre-#3856topojson files
7075d13
@etpinardetpinard mentioned this pull request Jul 15, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@archmojarchmojarchmoj left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugsomething brokenfeaturesomething new

Projects

None yet

Milestone

v1.48.0

Development

Successfully merging this pull request may close these issues.

2 participants

@etpinard@archmoj