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

Expose mapbox minzoom, maxzoom, and symbol-placement properties #3399

Merged
merged 4 commits into from
Feb 19, 2019

Conversation

rockg
Copy link
Contributor

@rockg rockg commented Jan 4, 2019

This exposes the mapbox minzoom, maxzoom, and symbol-placement properties. Currently this only exposes them to the mapbox argument and not to Scattermapbox.

Some items:

  1. It wasn't clear to me where tests in mapbox_test.js this should as I don't see other for strict mapbox arguments (most of the tests are for Scattermapbox).
  2. I followed the instructions for development on the Contributing page and step 4 tripped me up. The page opens, but there's nothing there despite the "mocks search" and the description suggests there should be tests in the dashboard.

@etpinard

@etpinard
Copy link
Contributor

Thanks very much for the PR!

I won't have the time to look at this in detail before we release 1.44.0 (in ~ 1 week from now). This does seem like a nice addition to the library!

@etpinard etpinard added this to the v1.45.0 milestone Feb 14, 2019
@etpinard
Copy link
Contributor

@plotly/plotly_js how do you feel about the proposed attributes:

  • layout.mapbox?.layers[i].(min|max)zoom
  • layout.mapbox?.layers[i].symbol.placement

Should we attempt to stay 1-to-1 with the mapbox-gl API or should we try to stay consistent with similar existing plotly.js attributes? I'm thinking

  • a more plotly.js-esque version of (min|max)zoom would be: zoomrange or just range
  • similarly placement -> position

Any opinions on this topic?

@alexcjohnson
Copy link
Collaborator

I'd vote to leave them as they are. (min|max)zoom is doing something very different from our other range-type attributes, and I imagine it's very common to only want to set one of them. placement is somewhat analogous to textposition, but seems to me it's unique enough that it will help people figure it out if its name matches where they should look in the mapbox docs.

@rockg
Copy link
Contributor Author

rockg commented Feb 15, 2019

Another thing that I can add here is some documentation to map the plot attributes to mapbox attributes. I didn't see this anywhere and it is not obvious without looking at the code the correspondence between the two. I can add this in the reference section for each attribute.

@etpinard
Copy link
Contributor

Another thing that I can add here is some documentation to map the plot attributes to mapbox attributes

Sure, go for it!


Moreover, could send us (better yet, check it into to test/image/mocks/) a "data" / "layout" JSON file that showcases these new attributes? Thanks!

@etpinard
Copy link
Contributor

We'll be making a minor release (v1.45.0) in the next few days - and it would be nice to include this PR in it.

@rockg Are you still planning on pushing another commit to this branch? If not, no worries, we'll gladly bring this PR over the finish line.

@rockg
Copy link
Contributor Author

rockg commented Feb 19, 2019

I should have time to do it by Wednesday and will let you know if I won't. Sorry for the delay.

].join(' ')
},
dash: {
valType: 'data_array',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does mapbox.layer.paint.line-dasharray also support scalars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Optional array of numbers greater than or equal to 0" here

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@etpinard
Copy link
Contributor

Thanks @rockg

I added the baseline to your new mock

image

Looking good!

@rockg
Copy link
Contributor Author

rockg commented Feb 19, 2019

@etpinard Thanks. Unfortunately the line label and min/max zoom aren't easily tested by the baseline as those are somewhat interactive.

@etpinard
Copy link
Contributor

Yeah, that's ok I guess. plotly.js is just passing things to mapbox-gl here. I'm sure mapbox has tests for those things.

Merging! Thank you very much for your contribution!

@etpinard etpinard merged commit 79fa245 into plotly:master Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants