Skip to content

Conversation

@yankev
Copy link
Contributor

@yankev yankev commented Feb 16, 2016

Changed the parameters for all the functions from taking on linkText and showLink to take on config (which is a dictionary of possible configurations).

yankev added 6 commits February 15, 2016 10:54
You can now enter in a dictionary for all the options you want using
the parameter ‘config’.
replaces all show_link,link_text with config
changed required parameters of all functions and updated the
descriptions.
@yankev
Copy link
Contributor Author

yankev commented Feb 16, 2016

@cldougl @chriddyp

'displaylogo',
'plotGlPixelRatio',
'setBackground',
'topojsonURL')
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we add another config option in plotly.js? This list can't stay in-sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cpsievert got this right, by not validating the config options: https://github.com/ropensci/plotly/pull/446/files

At some point, I want to make the config option part of the plot schema, where they could be use to validate user input from the python api. Before that happens though, I would much prefer not validating the config options.

@jackparmer
Copy link
Contributor

Related: #399


def iplot(figure_or_data, show_link=True, link_text='Export to plot.ly',
def iplot(figure_or_data,
config={'showLink': True, 'linkText': 'Export to plot.ly'},
Copy link
Member

Choose a reason for hiding this comment

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

we try really hard not to break backwards incompatibility. if we remove show_link and link_text then our user's code could break when they upgrade plotly. in this case, we might want to do something like:

iplot(figure_or_data, show_link=True, link_text='Export to plot.ly', **config_options)

@yankev
Copy link
Contributor Author

yankev commented Feb 18, 2016

Made the changes in a new branch, and create a new pull request here:
#410

@yankev yankev closed this Feb 18, 2016
@yankev yankev mentioned this pull request Feb 18, 2016
@nicolaskruchten nicolaskruchten deleted the issue399 branch June 19, 2020 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants