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

Add config options in plot schema #3376

Merged
merged 11 commits into from
Jan 16, 2019
Merged

Add config options in plot schema #3376

merged 11 commits into from
Jan 16, 2019

Conversation

etpinard
Copy link
Contributor

resolves #2703

Unlike #1188, this PR doesn't try to make the config options (or attributes?) compatible with Lib.coerce. Instead, this PR just lists valType and description fields for all the config options and formats them into the schema. Here, the default config "object" stays the same, hence there's no breakage in the behaviour.

Go to https://22282-45646037-gh.circle-artifacts.com/0/plot-schema.json and <ctrl-f> for "config": to see the results.

cc @plotly/plotly_js

@etpinard etpinard added this to the 1.44.0 milestone Dec 27, 2018
@etpinard
Copy link
Contributor Author

... @archmoj would you mind taking a look at this one at some point this week?

@archmoj
Copy link
Contributor

archmoj commented Jan 14, 2019

This one is also interesting to review. Thanks @etpinard.

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

Great PR.
I was wondering it may be useful for the users to have the plotly.js version written in the plot-schema.json.
Thanks @etpinard.

src/plot_api/plot_config.js Show resolved Hide resolved

queueLength: {
valType: 'integer',
min: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be useful to have a max for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently the Queue logic has no limit to the number of items.

You're right though, there should probably be a max, but as this is deprecated code, I'll pass.

].join(' ')
},

scrollZoom: {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may apply the changes to this attribute from other PR #3422:

scrollZoom: {
valType: 'flaglist',
flags: ['cartesian', 'gl3d', 'geo', 'mapbox'],
extras: [true, false],
dflt: 'gl3d+geo+mapbox',
description: [
'Determines whether mouse wheel or two-finger scroll zooms is enable.',
'Turned on by default for gl3d, geo and mapbox subplots',
'(as these subplot types do not have zoombox via pan),',
'but turned off by default for cartesian subplots.',
'Set `scrollZoom` to *false* to disable scrolling for all subplots.'
].join(' ')
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm planning on merging #3422 into this PR.

src/plot_api/plot_config.js Outdated Show resolved Hide resolved
src/plot_api/plot_config.js Outdated Show resolved Hide resolved
@etpinard
Copy link
Contributor Author

got a in-person 💃 from @archmoj - merging!

@etpinard etpinard merged commit cb9f9b0 into master Jan 16, 2019
@etpinard etpinard deleted the config-opts-in-plot-schema branch January 16, 2019 19:45
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.

Include valid config attributes with plot schema
2 participants