-
-
Notifications
You must be signed in to change notification settings - Fork 844
Generate the release cycle chart directly as SVG #1034
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
Conversation
Instead of Mermaid, use jinja2. Sphinx uses jinja for templating as well, so it might be possible to integrate this more tightly, but an include file works nicely for now. (It's actually easy to generate this chart just with f-strings, but I don't want to set a bad example for people who shouldn't fully trust their input. Jinja has autoescaping to prevent SVG injections.) Styling is done from the main stylesheet, and is bit more straightforward. I've adjusted the colours to be a bit friendlier to colour-blind people. There are vertical lines for all years now -- Mermaid's skipping of every other year was pretty confusing. Year labels have been shortened. This should work for another 10 years. The caption is removed; it was redundant in our case. Precise sizing is hard to do with SVG, but the font family, size and line height should nearly match the main text on big screens. At least in the current theme. In the code, `sorted_versions` is now a list, and the dicts in it have some extra generated info.
6d62d63
to
0e5ccfe
Compare
The current date line's position is now encoded in the SVG, rather than drawn by Javascript. With commits coming in every few weeks, it shouldn't matter. And if #998 gets in it'll ensure a commit every month or so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, while this is a lot of work and I'm sure you've done an excellent technical job, I would appreciate seeing a clear and concrete rationale for getting rid of the existing working mermaid approach and going with a handwritten SVG, given the amount of bespoke complexity it requires reviewing and maintaining ourselves indefinitely.
As to some possible reasons:
- It seems hard to justify this with the minuscule fraction of users who browse with JS fully disabled (those numbers often include those using NoScript, that can easily selectively enable it for things they want, which the
<noscript>
message makes clear), and given the same information is available in multiple other human and machine-readable forms. - Making the SVG sharable and embeddable seems a somewhat more compelling reason, but right now it's just being injected directly into the page at build time (otherwise it couldn't be styled with the CSS), so in practice it doesn't appear any more easily sharable or embeddable than it was before.
- There are some minor improvements to the appearance, but also a few rough edges that need polishing, and perhaps some of them could be made to the Mermaid version too.
Besides the complexity/maintenance issues mentioned above, unless and until #998 and #999 are implemented, it will only be updated ≈twice a year when the branches change to a different major status (and even with #998, it only adds a couple more updates; #999 would be required to update it monthly). I'm also not totally sure about the complexity of incorporating the information in #998, let alone #999 into it, as hand-writing SVG is not really in my wheelhouse.
That said, if we do go ahead with it, there are some things I noticed that should be fixed:
- The SVG template should be given an extension other than
SVG
(or similar), or configured with the appropriate.gitattributes
, as this currently makes it impossible to easily view it or the changes to it on GitHub due to it attempting and failing to render as an SVG image. - The "current date" line is shown behind all of release bars, making it much harder to see than the original. Could it be moved in front?
- To me, the inset text (particularly "security") actually looks harder to read than before with the drop shadow, even with the larger font size.
For the record, followup to #997 |
BenefitsBetter renderingThis new one renders much better on mobile:
Plus when rotating from portrait to landscape, no need to reload the page for it to re-render:
Better performance
This also benefits people with JavaScript enabled. It removes a 241 KB (compressed, 878K uncompressed) JS library resource. This makes the page and the site load much quicker. Currently the Mermaid JS is added to every page on the site, when we only need it for one page (I have an open PR to fix this: mgaitan/sphinxcontrib-mermaid#101). It's also quicker because instead of:
we're now:
To get some numbers, compare Google's PageSpeed Insights (aka Lighthouse):
This reduces: Time to Interactive, Blocking Time, Largest Contentful Paint The time spent evaluating JS is down from 1,167 ms -> 228 ms This "treemap" shows there's a lot less JS involved:
Other things
Sure, but no regression. Same also applies to the JSON. We can export them to a shareable location if we want (although we'd need to move some colours from CSS to the SVG for it to be standalone). And injecting into the page also removes an HTTP request.
👍 Complexity / maintenanceI'm going to be optimistic, it doesn't look too tricky? It's "just" some lines, rectangles and text boxes. Regular updates
It does seem a problem that the "now" line is static and only rarely updated. But can we re-generate the SVG during each deploy? Possibly de-version from Git, similar to the RSS in the PEPs repo. This repo is deployed several times a week. Re: #998 / #999, the Mermaid Gantt chart doesn't support additional sections in the same line. As noted in #998, that may also make the chart too cluttered, but we could try it out. If we decide to add extra charts instead, they shouldn't be too difficult, and templating can help if there's more than one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions.
(And okay to retain Black formatting on this file?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hugovk 's rationale seems compelling enough to me :)
Remaining issues not addressed by individual comments:
- The SVG template should be given an extension other than
SVG
(or similar), or configured with the appropriate.gitattributes
, as this currently makes it impossible to easily view it or the changes to it on GitHub due to it attempting and failing to render as an SVG image. - The "current date" line is shown behind all of release bars, making it much harder to see than the original. Could it be moved in front?
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Good idea, I renamed it to (Funnily, https://stackoverflow.com/q/29590931/724176 suggests both
Done! |
To avoid these CI warnings on Windows:
You might want to set might want to set
Meant to respond to this before, but yeah, I think that's the best approach, unless there's something I'm missing. AFAIK there's no actual need to actually check it in to the source other than it is built asynchronously from the rest of the docs and relied upon by them. So, now that we're only relying on Jinja (which is a Sphinx dep anyway, AFAIK) and it should be a fairly cheap operation, we could either just add the Other than these two issues and the fixed date comment above, LGTM! |
Done.
Also done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite neat!
Thanks for the reviews and to Petr for starting this off! |
Deployed: https://devguide.python.org/versions/ If you see something like this: Then do a hard reload (e.g. cmd-shift-r) to get load the new CSS: |
Easier said than done on mobile... The file that needs refreshing is https://devguide.python.org/_static/devguide_overrides.css https://docs.readthedocs.io/en/stable/hosting.html says Read the Docs busts the cache on the CDN when a new version is built, but looks like the max-age on that file is 86400 seconds (one day). I don't see an easy way to force it to clear, short of renaming the file in Git, so guess we should just wait. (Or have users go into browser settings and clear the cache...) |
When testing the PEPs banner PR, press and holding on the reload button would trigger a hard reload in the browsers I tried, or at least appeared to.
What static site generators typically do is embed a hash of the source asset as a dummy query string in the asset url that changes when the file is updated, avoiding caching. Lektor, for example, as an asseturl Jinja filter that does this. Not sure if Sphinx has something similar; this would have to be done in the theme, of course. @pradyunsg , ideas? |
Short term: good news, the cache has now expired, I see the chart properly on my phone. Longer term: It's good Furo includes a digest hash for its resources: <link rel="stylesheet" type="text/css" href="../_static/pygments.css" />
<link rel="stylesheet" type="text/css" href="../_static/styles/furo.css?digest=91d0f0d1c444bdcb17a68e833c7a53903343c195" />
<link rel="stylesheet" type="text/css" href="../_static/copybutton.css" />
<link rel="stylesheet" type="text/css" href="../_static/styles/furo-extensions.css?digest=30d1aed668e5c3a91c3e3bf6a60b675221979f0e" />
<link rel="stylesheet" type="text/css" href="../_static/devguide_overrides.css" /> But Sphinx doesn't for our CSS file: html_css_files = [
'devguide_overrides.css',
] Or the other CSS files. Maybe this could be improved in Sphinx? Maybe there's a bug in RTD not busting the cache properly? |
sphinx-doc/sphinx#11133 is the Sphinx-side request to allow for this. |
How's this for procrastination:

Please amend this PR if you want (and can); I might not get to procrastinate more soon.
Instead of Mermaid, use jinja2. Sphinx uses jinja for templating as well, so it might be possible to integrate this more tightly, but an include file works nicely for now.
(It's actually easy to generate this chart just with f-strings, but I don't want to set a bad example for people who shouldn't fully trust their input. Jinja has autoescaping to prevent SVG injections.)
Styling is done from the main stylesheet, and is bit more straightforward. I've adjusted the colours to be a bit friendlier to colour-blind people.
The labels on the boxes aren't the greatest, but I expect them to go away after #998 -- ideally replaced by a legend at the bottom.
There are vertical lines for all years now -- Mermaid's skipping of every other year was pretty confusing. Year labels have been shortened. This should work for another 10 years.
The caption is removed; it was redundant in our case.
Precise sizing is hard to do with SVG, but the font family, size and line height should nearly match the main text on big screens. At least in the current theme.
In the code,
sorted_versions
is now a list, and the dicts in it have some extra generated info.