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

Override toImage modebar button opts via config #2607

Merged
merged 3 commits into from
May 3, 2018

Conversation

nicolaskruchten
Copy link
Contributor

This PR allows for selective overriding of default behaviour of the toImage modebar button. Specifying nothing falls through to the current defaults. Note sure about the implications of renaming the button, wrt to locale files.

Copy link
Contributor

@etpinard etpinard left a comment

Choose a reason for hiding this comment

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

Looks good!

The only thing missing is a test. Our modebar suite is a bit of a mess unforunately and testing image downloads in jasmine isn't great. But we should still try!

So I'm thinking adding a new describe block in modebar_test.js that sets spyOn(Registry, 'call') and look up its arguments after click (using toHaveBeenCalledWith I think) for different config options.

}

Registry.call('downloadImage', gd, {'format': format})
if(toImageButtonDefaults.width) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. No need for isNumeric() here toImage handles it already:

var format = coerce('format');
var width = coerce('width');
var height = coerce('height');
var scale = coerce('scale');
var setBackground = coerce('setBackground');
var imageDataOnly = coerce('imageDataOnly');

}
if(toImageButtonDefaults.filename) {
opts.filename = toImageButtonDefaults.filename;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolaskruchten we might as well add scale

scale: {
valType: 'number',
min: 0,
dflt: 1,
description: [
'Sets a scaling for the generated image.',
'If set, all features of a graphs (e.g. text, line width)',
'are scaled, unlike simply setting',
'a bigger *width* and *height*.'
].join(' ')
},

@etpinard etpinard added this to the v1.38.0 milestone May 3, 2018
@nicolaskruchten nicolaskruchten changed the title WIP override toImage modebar button opts via config verride toImage modebar button opts via config May 3, 2018
@nicolaskruchten nicolaskruchten changed the title verride toImage modebar button opts via config Override toImage modebar button opts via config May 3, 2018
@etpinard
Copy link
Contributor

etpinard commented May 3, 2018

Nicely done. 💃

@etpinard
Copy link
Contributor

etpinard commented May 3, 2018

(unless someone would prefer another name for that config key instead of toImageButtonDefaults)


Lib.notifier(_(gd, 'Taking snapshot - this may take a few seconds'), 'long');

if(Lib.isIE()) {
Lib.notifier(_(gd, 'IE only supports svg. Changing format to svg.'), 'long');
format = 'svg';
opts.format = 'svg';
Copy link
Collaborator

Choose a reason for hiding this comment

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

also we should change the conditional to if(Lib.isIE() && opts.format !== 'svg')

@@ -47,19 +47,26 @@ var modeBarButtons = module.exports = {};

modeBarButtons.toImage = {
name: 'toImage',
title: function(gd) { return _(gd, 'Download plot as a png'); },
title: function(gd) { return _(gd, 'Download plot'); },
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can't make dynamic strings here, so _(gd, 'Download plot as a ' + format) won't work, but perhaps we can keep 'Download plot as a png' as the default string and only degrade to 'Download plot' if it's not a png? Two reasons:

  • I don't want to unnecessarily break all our existing translations, for folks who are not using this new option.
  • I do find "as a png" helpful, to clarify that it's not the data or json for example, though I guess the icon mitigates that substantially.

@alexcjohnson
Copy link
Collaborator

(unless someone would prefer another name for that config key instead of toImageButtonDefaults)

They're not really defaults at that point, since there's no way to override them further, right? So I might vote toImageButtonOptions or something.

@nicolaskruchten
Copy link
Contributor Author

Thanks @alexcjohnson ! I've implemented your suggestions

@alexcjohnson
Copy link
Collaborator

Thanks! 💃

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.

3 participants