Skip to content

DOC Improve resizing of plotly parallel coord plots #30778

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

Merged
merged 3 commits into from
Feb 6, 2025

Conversation

jeromedockes
Copy link
Contributor

@jeromedockes jeromedockes commented Feb 6, 2025

Improvement of #30297

As discussed with @glemaitre and @lesteve :

I tried to use the strategy used to resize plotly figures in the example gallery for the skrub library, however it was failing when I had several parallel plots in the same example

in skrub I changed the script as shown in this PR:

the figures are already responsive by default, so we don't need to resize them manually using Plotly.Plots.resize (nor to listen to resize events). It seems to be enough to trigger a resize event once the DOM has loaded and the figures will adjust themselves.

But I haven't tried to reproduce the issue I had in a scikit-learn example so actually I don't know if the problem would occur or if this change would help 🤷

Copy link

github-actions bot commented Feb 6, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 43868cf. Link to the linter CI: here

@lesteve lesteve changed the title different way of resizing plotly parallel coord plots DOC improve resizing of plotly parallel coord plots Feb 6, 2025
Copy link
Member

@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

LGTM, I pushed a commit try to trigger a CircleCI run on an example using parallel coordinate plot to double-check this works.

@lesteve
Copy link
Member

lesteve commented Feb 6, 2025

Rendered documentation looks fine, compared to 1.5 doc which has the rendering issue (colorbar of the parallel coordinate plot is only partly visible)

@lesteve lesteve enabled auto-merge (squash) February 6, 2025 18:29
@lesteve
Copy link
Member

lesteve commented Feb 6, 2025

I reverted my temporary change and set auto-merge

@lesteve lesteve changed the title DOC improve resizing of plotly parallel coord plots DOC Improve resizing of plotly parallel coord plots Feb 6, 2025
@lesteve lesteve disabled auto-merge February 6, 2025 19:06
@lesteve lesteve enabled auto-merge (squash) February 6, 2025 19:06
@lesteve lesteve merged commit 4f29d4e into scikit-learn:main Feb 6, 2025
30 checks passed
@jeromedockes jeromedockes deleted the resizing-plotly-figures branch February 7, 2025 08:38
@lesteve
Copy link
Member

lesteve commented Feb 7, 2025

@jeromedockes out of curiosity, it feels like the fix would belong in pydata-sphinx-theme, that would need to trigger a resize event once it is loaded. Would you agree?

@jeromedockes
Copy link
Contributor Author

jeromedockes commented Feb 7, 2025 via email

@jeromedockes
Copy link
Contributor Author

@lesteve I think we can follow this issue: plotly/plotly.js#7059

it is also suggesting to watch parent element size changes with ResizeObserver and adjust the plots. If / when it gets closed we can remove the script from the sickit-learn doc config

@jeromedockes
Copy link
Contributor Author

OTOH I guess many pydata-themed docs use plotly so it might make sense to have the quickfix in the theme until the issue is fixed in plotly as you suggested 🤔

@lesteve
Copy link
Member

lesteve commented Feb 7, 2025

Thanks a lot for the investigation, I have added a comment in plotly: plotly/plotly.js#7059 (comment). Let's see what they say!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants