The Wayback Machine - https://web.archive.org/web/20220519204024/https://github.com/plotly/plotly.js/pull/5209
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
Member

@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 Oct 10, 2020
@nicolaskruchten
Copy link
Member Author

@nicolaskruchten nicolaskruchten commented Oct 10, 2020

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
Member Author

@nicolaskruchten nicolaskruchten commented Oct 10, 2020

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
Copy link
Contributor

@archmoj archmoj commented Oct 14, 2020

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

@chriddyp
Copy link
Member

@chriddyp chriddyp commented Oct 15, 2020

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

@nicolaskruchten
Copy link
Member Author

@nicolaskruchten nicolaskruchten 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 :)

@archmoj
Copy link
Contributor

@archmoj 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
Member Author

@nicolaskruchten nicolaskruchten commented Oct 15, 2020

@archmoj
Copy link
Contributor

@archmoj 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
Member Author

@nicolaskruchten nicolaskruchten Oct 15, 2020

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
Member Author

@nicolaskruchten nicolaskruchten Oct 15, 2020

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

@archmoj archmoj Oct 15, 2020

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
Member Author

@nicolaskruchten nicolaskruchten commented Oct 15, 2020

💃 ! (see my one question above!)

@archmoj archmoj merged commit 3580851 into master Oct 15, 2020
9 checks passed
@archmoj archmoj deleted the nicolaskruchten-patch-3 branch Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment