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.

Sign up for GitHub

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

Jump to bottom

Adding plotly support to gallery examples. #76

Closed
wants to merge 2 commits into from
Closed

Adding plotly support to gallery examples. #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dec3475
Updating layout
germa89 Jun 14, 2022
3878709
Adding if condition to avoid include plotly in all the packages.
germa89 Jun 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/ansys_sphinx_theme/layout.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
{# Append our custom CSS after the bootstrap stylesheet so we can override where necessary #}
{%- block css %}
{{ super() }}
{# adding support for plotly in gallery examples #}
Copy link
Member

Choose a reason for hiding this comment

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

Can we hide this behind a feature flag in the conf.py? It's adding 3.3MB to the page size, and presumably eats some performance on page load to execute the JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a very valid point.

I could create a flag in the conf.py file, so it will load it only if activated. Would it work for you?

(btw, who checks the size of the packages?? hahaah #Joking)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thought the same. This basically requires plotly for each and every page.

I think the best approach is to run a "cleanup" script that simply swaps the position of the two lines while submitting an issue (and ideally PR) to the sphinx-gallery team (or applicable repo) to clean up the actual issue.

Copy link
Member

Choose a reason for hiding this comment

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

I could create a flag in the conf.py file, so it will load it only if activated. Would it work for you?

Yep, that would be great! I suspect plotly will be in relatively common use, but for some of the simpler pages it would be nice not having to load it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not something like:

{# adding support for plotly in gallery examples #}
    {% if theme_use_plotly %}
        <script src="https://cdn.plot.ly/plotly-latest.min.js"></script>
    {% endif %}

and then add:

use_plotly=True

to the theme.conf file???

Copy link
Member

Choose a reason for hiding this comment

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

@akaszynski is your concern that this still loads on every page of the documentation (if enabled)? In terms of size I think that's less of a concern (after it's loaded once it should be cached?), but still needs to execute the JS (take that with a grain of salt, I'm not an expert on these things).

Of course, if we can fix it "upstream" that would be the ideal solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akaszynski is your concern that this still loads on every page of the documentation (if enabled)? In terms of size I think that's less of a concern (after it's loaded once it should be cached?), but still needs to execute the JS (take that with a grain of salt, I'm not an expert on these things).

Of course, if we can fix it "upstream" that would be the ideal solution.

Should have been clearer.

I'm no expert either, but and I'm assuming that it's using the cache. Still, it appears the JS still is executed/loaded.

pydata-sphinx-theme won't fix the issue. As far as I can tell, it's in:
https://github.com/jupyter-widgets/ipywidgets/blob/9326d28c474cb6d2b58876d469489a73e98f903a/python/ipywidgets/ipywidgets/embed.py#L244-L319

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not something like:

{# adding support for plotly in gallery examples #}
    {% if theme_use_plotly %}
        <script src="https://cdn.plot.ly/plotly-latest.min.js"></script>
    {% endif %}

and then add:

use_plotly=True

to the theme.conf file???

That will still require that each and every page executes the plotly JS regardless of if it's used. Still feels like a bad solution relative to simply patching the HTML until it's fixed upstream.

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'm not expert either... @akaszynski should I take the time to propose something in ipywidgets repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Try first with the sphinx-gallery

{% if theme_use_plotly %}
<script src="https://cdn.plot.ly/plotly-latest.min.js"></script>
{% endif %}
{% if theme_show_breadcrumbs %}
<link href="{{ pathto('_static/css/breadcrumbs.css', 1) }}" rel="stylesheet">
{% endif %}
Expand Down
1 change: 1 addition & 0 deletions src/ansys_sphinx_theme/theme.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ show_breadcrumbs = True
show_icons = True
hidden_icons =
additional_breadcrumbs =
use_plotly=True