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

Force default dims for downloadImage to null #5209

Merged
merged 2 commits into from
Oct 15, 2020
Merged

Conversation

nicolaskruchten
Copy link
Contributor

@nicolaskruchten nicolaskruchten commented Oct 10, 2020

Another variation on #5205 to address the class of issue of which #5202 is an exemplar ... closes #1576

@nicolaskruchten nicolaskruchten marked this pull request as ready for review October 10, 2020 16:52
@nicolaskruchten
Copy link
Contributor Author

OK we probably need a test here but I think this is the behaviour I want @alexcjohnson :)

@antoinerg will this have some impact on the full-json image export format somehow, or does that use toImage directly? Does Kaleido use downloadImage @jonmmease ?

@nicolaskruchten
Copy link
Contributor Author

OK so Kaleido uses the underlying toImage which isn't modified here: https://github.com/plotly/Kaleido/blob/7de733fe4bae03bb8b9957b3c130c621777df9bf/repos/kaleido/js/src/plotly/render.js#L129

The only cases I can think of where this isn't a desireable change is in a Dash app where there is a quite-small chart and people are using the download button to get a bigger version. @chriddyp is this a reason not to do this? This PR essentially fixes plotly/dash-core-components#199 more fully than #3746 did by making it the default.

@archmoj archmoj added status: reviewable bug something broken labels Oct 14, 2020
@archmoj
Copy link
Contributor

archmoj commented Oct 14, 2020

Also tagging @wbrgss in case there is any limitations that he knows of?

@chriddyp
Copy link
Member

Seems reasonable 👍 as long as it's overridable for default dimensions (and documented)

@nicolaskruchten
Copy link
Contributor Author

OK, great! @archmoj once 1.57 is out, please lock this behaviour down with a test and let's merge it :)

@archmoj
Copy link
Contributor

archmoj commented Oct 15, 2020

OK, great! @archmoj once 1.57 is out, please lock this behaviour down with a test and let's merge it :)

Do you want it to be in 1.57.0?

@nicolaskruchten
Copy link
Contributor Author

nicolaskruchten commented Oct 15, 2020 via email

@archmoj
Copy link
Contributor

archmoj commented Oct 15, 2020

The test is added by 5e98788.
Now tagging this in v1.57.0.

@archmoj archmoj added this to the v1.57.0 milestone Oct 15, 2020
gd.style.width = '500px';
gd.style.height = '300px';

Plotly.plot(gd, fig)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume this test would fail without my change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💃 if so!

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this test would fail without my change?

Yes it used to fail without the changes proposed by you.
Thanks very much @nicolaskruchten for fixing this bug!

@nicolaskruchten
Copy link
Contributor Author

💃 ! (see my one question above!)

@archmoj archmoj merged commit 3580851 into master Oct 15, 2020
@archmoj archmoj deleted the nicolaskruchten-patch-3 branch October 15, 2020 16:52
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
3 participants