Upgrade d3 geo projections #5112

archmoj · 2020-09-01T03:34:40Z

Replace old d3.geo functions with upgraded d3-geo modules (that support IE) as part of d3 bump cc: #424.
Some new geo projections e.g. satelite could potentially be added as listed in 8dd4ba0 to address #4781.

This would also result in reducing the size of various partial bundles that do not need geo trace.

@plotly/plotly_js

alexcjohnson · 2020-09-01T21:56:36Z

This looks like a good idea, but I think we need to be more careful about each projection we add: (1) does it work well in the simple case? In your demo codepen a number of them fall look pretty bad either immediately or easily as you zoom / pan around so should probably not be included without some adjustment. (2) maybe you've already done this, do they support all the same parameters as the existing ones? isConic? lonaxisSpan / lataxisSpan? Any other special cases? (3) each one we add needs to be included in at least one mock, maybe more so we can ensure their parameters work right. geo_fitbounds-locations says it includes "all projection types", let's ensure that stays true.

- note upgrading to latest versions is not possible atm - due to ES2015 syntax and drop of IE support

archmoj · 2021-01-14T16:31:16Z

package.json

@@ -57,7 +57,7 @@
    ]
  },
  "dependencies": {
-    "@plotly/d3": "^3.5.18",
+    "@plotly/d3": "git://github.com/plotly/d3.git#e792de9ae52d956d18582cac8a49ea6a03c7e010",


plotly/d3@master...plotly:drop-geo

archmoj · 2021-01-14T16:33:48Z

This looks like a good idea, but I think we need to be more careful about each projection we add: (1) does it work well in the simple case? In your demo codepen a number of them fall look pretty bad either immediately or easily as you zoom / pan around so should probably not be included without some adjustment. (2) maybe you've already done this, do they support all the same parameters as the existing ones? isConic? lonaxisSpan / lataxisSpan? Any other special cases? (3) each one we add needs to be included in at least one mock, maybe more so we can ensure their parameters work right. geo_fitbounds-locations says it includes "all projection types", let's ensure that stays true.

Thanks for the notes @alexcjohnson.
With the updates (reflected in the PR description now) no new projections were added in the API as a result of this PR.

alexcjohnson · 2021-01-14T18:10:14Z

test/jasmine/tests/geo_test.js

@@ -1353,46 +1353,6 @@ describe('Test geo interactions', function() {
        .then(done, done.fail);
    });

-    it('should clear hover label when cursor slips off subplot', function(done) {


Why did these 2 tests get removed?

If I recall (as mentioned in the commit message) there used to be a bug in d3.geo where a function returned NaN positions. And we guarded against it.

But these tests are just using the high-level API, not d3 geo itself. If they started failing, that means something changed about the user-facing behavior. What changed? Are we comfortable with that change? I'd much prefer if we can adapt the tests to the new version so we can see the difference.

Good question. Pointing to the fact that rebase somehow forgot to remove geo/projections.
It is now fixed in 557ebde.

This test should still be able to pass though. Perhaps the exact pixel positions need to change because there's some small change in how the projection is applied? If I try to manually follow what's going on in the test (I believe it's hovering on the point off the coast of Africa, then slowly mousing off the edge of the globe) the behavior looks identical before/after the change. So we should be able to bring the test back, but if the exact values need to change a little (px = 390, py = 290 and if(px < 402) that's fine.

Good call. Adjusted in 647b0dc.
In fact with previous positions the tests didn't pass locally. But now the test now passes both locally and on the CI.

nicolaskruchten · 2021-01-14T18:54:48Z

We don't strictly speaking need to merge this PR for v2, so let's leave it alone for now?

archmoj · 2021-01-14T19:14:30Z

We don't strictly speaking need to merge this PR for v2, so let's leave it alone for now?

That's what we may end up doing. But it is worth having the conversion going before committing to v2 IMHO.

… functions" This reverts commit a7735e6.

src/plots/geo/geo.js

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

archmoj · 2021-06-26T15:43:48Z

Conflicts are resolved here and the plotly/d3 PR.
Now adding this to v2.3.0 milestone.

alexcjohnson

Looks good, thanks for making the new issue.
💃 after publishing the updated @plotly/d3

package.json

archmoj added type: feature status: reviewable type: bug labels Sep 1, 2020

archmoj added status: has TODOs and removed status: reviewable labels Sep 24, 2020

archmoj force-pushed the new-d3-geo branch from b44e7fa to 7a11695 Compare December 3, 2020 14:58

archmoj force-pushed the new-d3-geo branch from 7a11695 to 59900dc Compare January 5, 2021 17:19

apply latest d3-geo v1 and d3-geo-projection v2

9f679fd

- note upgrading to latest versions is not possible atm - due to ES2015 syntax and drop of IE support

archmoj force-pushed the new-d3-geo branch from 59900dc to 8dd4ba0 Compare January 14, 2021 16:22

archmoj added 4 commits January 14, 2021 11:23

update two baselines

f17f3bf

remove tests related to unhandled bugs in the previous d3-geo functions

a7735e6

link to d3 v3 without geo

7730d43

add comment about the potential projections to add

8dd4ba0

archmoj added Major change status: reviewable and removed status: has TODOs labels Jan 14, 2021

archmoj added this to the NEXT milestone Jan 14, 2021

archmoj commented Jan 14, 2021

View reviewed changes

archmoj requested a review from alexcjohnson January 14, 2021 16:34

Merge remote-tracking branch 'origin' into new-d3-geo

7e36f6c

alexcjohnson reviewed Jan 14, 2021

View reviewed changes

remove unused geo/projections.js file

557ebde

archmoj removed the Major change label Jan 14, 2021

archmoj removed this from the NEXT milestone Jan 14, 2021

This was referenced Jan 16, 2021

First attempt at a click anywhere #5416

Closed

Include calendars in all bundles #5422

Merged

archmoj added 2 commits January 20, 2021 19:10

Revert "remove tests related to unhandled bugs in the previous d3-geo…

f46e4af

… functions" This reverts commit a7735e6.

no longer need to add fitExtend from d3 v4

d5d352c

archmoj added status: has TODOs and removed status: reviewable labels Jan 21, 2021

archmoj mentioned this pull request Feb 22, 2021

Revise process of making bundles and simplify making partial bundles #5508

Merged

archmoj mentioned this pull request May 3, 2021

Fix geo plot hover interaction for Firefox #5607

Merged

archmoj added 4 commits May 22, 2021 14:16

Merge remote-tracking branch 'origin/master' into new-d3-geo

b9be8c1

no longer need to check for NaN with d3-geo

d5883d2

adjust geo test - previous test used to fail locally on master too

647b0dc

clear fallback to default view - no longer NaNs in d3-geo

a046871

archmoj added status: reviewable and removed status: has TODOs type: bug type: feature labels May 22, 2021

alexcjohnson reviewed May 24, 2021

View reviewed changes

src/plots/geo/geo.js Outdated Show resolved Hide resolved

archmoj and others added 3 commits May 25, 2021 11:24

fixup geo requires

98c19b8

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>

Merge remote-tracking branch 'origin/master' into new-d3-geo

905eeb0

regenerate two baselines using kaleido

0a7ed9f

archmoj added this to the v2.3.0 milestone Jun 26, 2021

archmoj added type: bug type: maintenance labels Jun 26, 2021

archmoj mentioned this pull request Jun 29, 2021

Investigate fixing scroll zoom handler after switching to more recent d3-geo #5787

Open

alexcjohnson approved these changes Jun 30, 2021

View reviewed changes

archmoj mentioned this pull request Jun 30, 2021

Retire geo now that d3-geo module could be used with plotly.js plotly/d3#3

Merged

archmoj commented Jun 30, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

archmoj added 2 commits June 30, 2021 09:36

use plotly/d3 v3.7.0 which no longer has geo part

be527cf

Merge remote-tracking branch 'origin/master' into new-d3-geo

32aeac6

archmoj merged commit 17c9f0c into master Jun 30, 2021

archmoj deleted the new-d3-geo branch June 30, 2021 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade d3 geo projections #5112

Upgrade d3 geo projections #5112

archmoj commented Sep 1, 2020 •

edited

alexcjohnson commented Sep 1, 2020

archmoj Jan 14, 2021

archmoj commented Jan 14, 2021

alexcjohnson Jan 14, 2021

archmoj Jan 14, 2021

alexcjohnson Jan 14, 2021

archmoj Jan 14, 2021

alexcjohnson Jan 20, 2021

archmoj May 22, 2021

nicolaskruchten commented Jan 14, 2021

archmoj commented Jan 14, 2021

archmoj commented Jun 26, 2021

alexcjohnson left a comment

Upgrade d3 geo projections #5112

Upgrade d3 geo projections #5112

Conversation

archmoj commented Sep 1, 2020 • edited

alexcjohnson commented Sep 1, 2020

archmoj Jan 14, 2021

Choose a reason for hiding this comment

archmoj commented Jan 14, 2021

alexcjohnson Jan 14, 2021

Choose a reason for hiding this comment

archmoj Jan 14, 2021

Choose a reason for hiding this comment

alexcjohnson Jan 14, 2021

Choose a reason for hiding this comment

archmoj Jan 14, 2021

Choose a reason for hiding this comment

alexcjohnson Jan 20, 2021

Choose a reason for hiding this comment

archmoj May 22, 2021

Choose a reason for hiding this comment

nicolaskruchten commented Jan 14, 2021

archmoj commented Jan 14, 2021

archmoj commented Jun 26, 2021

alexcjohnson left a comment

Choose a reason for hiding this comment

archmoj commented Sep 1, 2020 •

edited