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

Fix lakes and rivers geometry on scoped geo subplots #4048

Merged
merged 4 commits into from
Jul 16, 2019

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Jul 15, 2019

fixes #4046 (a regression from #3856) with the help of etpinard/sane-topojson#14

@etpinard etpinard added status: reviewable bug something broken labels Jul 15, 2019
@etpinard etpinard added this to the v1.49.0 milestone Jul 15, 2019
@etpinard
Copy link
Contributor Author

cc @plotly/plotly_js don't attempt to update https://codepen.io/nicolaskruchten/pen/orRBKb?editors=0010 from #4046 to see the new behaviour. We need to first update https://cdn.plot.ly/africa_110m.json and friends.

@etpinard
Copy link
Contributor Author

You'll notice that scope: 'europe' is also currently having issues with showlakes:true.

@@ -0,0 +1,264 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This mock currently looks like:

newplot (42)

Copy link
Contributor Author

@etpinard etpinard Jul 15, 2019

Choose a reason for hiding this comment

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

Using topojson files from before #3856 fixes the lake polygons, but the scoped rivers were still off, see 7075d13 or diff in 5385673

@archmoj
Copy link
Contributor

archmoj commented Jul 15, 2019

It is rather difficult to view & compare the before/after images (with aspect-ratio close to 2) on small monitors. @etpinard would you mind splitting the mock into two mocks?

@etpinard
Copy link
Contributor Author

@archmoj does this help?

Peek 2019-07-15 15-55

@archmoj
Copy link
Contributor

archmoj commented Jul 15, 2019

@etpinard I cannot swipe over the center (mid-height) of the image.
Anyway that's just fine.

@antoinerg
Copy link
Contributor

Thank you @etpinard for the update and the mock

💃

@etpinard
Copy link
Contributor Author

Is @archmoj ok also with the changes?

Here's the script I used to generate the new mock:

var schema = Plotly.PlotSchema.get()
var scopes = schema.layout.layoutAttributes.geo.scope.values
var resolutions = [110, 50]

var traces = []
var layout = {
  grid: {rows: scopes.length, columns: resolutions.length},
  width: 600,
  height: 1500
}
var k = 0

resolutions.forEach((r, i) => {
scopes.forEach((s, j) => {
  var id = 'geo' + (k ? (k + 1) : '')
  
  traces.push({type: 'scattergeo', geo: id})
  layout[id] = {
    scope: s,
    resolution: r,
    domain: {column: i, row: j},
    landcolor: "brown",
    showland: true,
    lakecolor: "blue",
    showlakes: true,
    rivercolor: "blue",
    showrivers: true
  }
  
  k++
})
})

Plotly.newPlot(gd, traces, layout, {scrollZoom:false})

It should be easy to transpose (i,j) -> (j,i), if that makes it easier to review for you.

@archmoj
Copy link
Contributor

archmoj commented Jul 16, 2019

💃

@etpinard etpinard merged commit 218a919 into master Jul 16, 2019
@etpinard etpinard deleted the fixup-geo-scope-lake-polygons branch July 16, 2019 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Countries missing in scope=africa
3 participants