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
Furo docs theme with styling #23159
Furo docs theme with styling #23159
Conversation
✅ Hi, I am the SymPy bot (v163). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.11. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
🟠Hi, I am the SymPy bot (v163). I've noticed that some of your commits add or delete files. Since this is sometimes done unintentionally, I wanted to alert you about it. This is an experimental feature of SymPy Bot. If you have any feedback on it, please comment at sympy/sympy-bot#75. The following commits add new files:
The following commits delete files:
If these files were added/deleted on purpose, you can ignore this message. |
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[77f1d79c] [459a2aa4]
<sympy-1.10.1^0>
+ 98.7±0.6ms 178±0.8ms 1.80 sum.TimeSum.time_doit
Full benchmark results can be found as artifacts in GitHub Actions |
Note: we should probably rebase away the submodule addition before merging any of this. |
I've uploaded a demo site so you don't have to build this yourself https://github.com/asmeurer/sympy-furo-demo |
Thanks @asmeurer. To save folks a click, the built site is at https://www.asmeurer.com/sympy-furo-demo/ |
Great start! Comments:
|
Sure that's doable. I'm not sure how good it would look. Having it blend in does make it harder to see. I guess the flip side of that is that it's less distracting. The current docs site has the search bar white https://docs.sympy.org/dev/index.html.
Yeah, I noticed some of these things as well (the Chrome inspect accessibility thing flagged them). I'm a little unsure how to fix some of them. A problem I noticed with these color contrast checkers is that they only consider the contrast of the text against its background. They don't consider the contrast of the text with the surrounding text. This is particularly important for the link color. We can make the (currently green) links have a better contrast against the background by making them darker, but then they become harder to see as links against the surrounding black text (you can see what I mean if you right click "inspect" a link in Chrome and then click the color in the inspector and choose the suggested higher contrast color). I can try flipping the dark and light greens to see how it looks. I was also planning on adjusting the code block background (which should be easier than trying to restyle all the different syntax highlighting colors). The problem is the Chrome dev tools only support suggesting foreground colors, not background colors. But I did find https://accessible-colors.com/ which allows both. AAA is hard to achieve while still keeping distinguishable colors, but we should be able to get AA I think. I also tried just messing with the color picker, but I had a hard time finding something I liked aesthetically. I do personally like how the code blocks stand out as I think the examples are the most important part of documentation. |
doc/src/conf.py
Outdated
|
||
html_theme_options = { | ||
"light_css_variables": css_settings_common, | ||
"dark_css_variables": {**css_settings_common, |
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.
You shouldn't need this -- the light mode variables get inherited for dark mode.
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.
See https://pradyunsg.me/furo/customisation/colors/#how-light-and-dark-mode-work for the relevant details.
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.
I didn't realize this. It would be nice if this were mentioned at https://pradyunsg.me/furo/customisation/#light-css-variables-dark-css-variables
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 was working, but I updated to the latest Furo and it stopped working and I had to separate them again.
Relevant here: https://pradyunsg.me/furo/customisation/sidebar/. See also pradyunsg/furo#372. |
If you scroll through the api listing, the function names should be the only thing you can detect while scrolling (or the primary thing). |
Thanks for this. The new docs site looks really nice! |
Addressed some comments and additional things I noticed:
I had one idea also, which is to make the function names easier to "spot" on the page by inverting the colors to make the background green (it is currently grey), and the text black (or some other color that's easy to read on green). I'm a little unsure about it, though. Let me know if you think it would be better that way or the way it is now. |
Another note: we are doing enough modifications in custom.css that it might be a good idea to pin the Furo version. We can update it every once in a while, but whenever we do, we have to check if any CSS modifications are required. Even without custom.css, after updating my local version Furo, there were some additional "supported" CSS variables added that required updating. |
Here's a quick mockup of what I mean. Right now, it looks like this: I'm wondering if we should instead change it to something like this: Or would that be too noisy on the page? It would only apply to function and class definitions in the API docs, not other headers. |
Thanks for doing all this detailed work, @asmeurer. I do find the second version visually noisy, as I prefer blocks of color to be subtle; that also makes it easier to achieve sufficient color contrast. How about largely keeping the first version, and making the module prefix names text the darker green? The actual function name could be bold to distinguish it from the module prefixes. |
Pushed a few more fixes:
I'm honestly still not sure how to handle the lighthouse contrast issues in light mode. Unlike dark mode, most of the suggestions for contrast improvements in light mode seem to make the text too close to black to be distinguishable. Is there an easy way to run lighthouse on every page, with both light and dark mode? I've been running it one page at a time, but different pages have different elements. There are way too many pages to run each page manually. |
I like both, definitely fixes the issues I reaised. Thanks! For the second, the green bars are strong. Maybe a lighter shade of green would be better. The first one is good too though, and sufficient. |
Lighthouse can be run via npm; I haven't tried that. I'm not sure if there's a way to automatically do that in both light and dark mode, as the URL doesn't change when I click dark mode. The mode selection does persist when you go from one page to another, so perhaps you could run the Lighthouse scans in light mode, then change to dark mode, then re-running the Lighthouse scans might use the dark mode? I'm not sure if that mode will be picked up by the npm version of Lighthouse. |
It looks like you can force dark mode with the |
I was thinking it'd be nice to temporarily add headers to each page (in both released and dev docs) when we want to publicize a survey or the like to the community--that would reach anyone who uses the docs, not just people who follow SymPy on Github, Twitter, Gitter, etc. Is there an easy way to do that? |
I think Lighthouse only works if the page is posted on a server. I wonder if there's a way to give it a sitemap to have it visit all the mapped pages? |
The announcement feature makes it quite easy to add it to the dev docs. To add it to the "latest" docs we would need to manually rebuild the docs from that git tag with the given header (which isn't too hard). |
Some updates
This is now ready for review. I have made all the style changes I wanted to make, so please let me know if you see anything that you feel should be changed in any way. Also if anyone is knowledgeable about CSS or Javascript I would appreciate a review of my CSS in |
doc/src/_static/custom.css
Outdated
/* Make the "class", "property" and so on before function names in the API*/ | ||
/* docs bold. */ | ||
em.property:first-child { | ||
font-weight: bold; | ||
} |
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.
The entire signature is bold by default, so you shouldn't need this.
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.
Looks like you are right. This must have been left over from a previous iteration.
} | ||
--> | ||
<div class="sidebar-tree sidebar-versions"> | ||
<h5 class="versions-header">Documentation Version</h4> |
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.
I would suggest adding a padding-left to this, to align it with the rest of the sidebar content. You might want to consider if you want this to look like sidebar captions instead of a heading.
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.
I'll have to play with it some more. I'm OK with the way it currently looks but I'll see if it looks better this way. Note that I'm not a designer and my HTML/Javascript/CSS skills are terrible, so if you see something that looks off, it's most likely unintentional so please let me know.
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.
Actually this was supposed to have the exact same padding
as the rest of the sidebar, but my editor inserted an incorrect line break in the CSS and messed it up. Should look better now.
"light_css_variables": common_theme_variables, | ||
# The dark variables automatically inherit values from the light variables | ||
"dark_css_variables": { | ||
**common_theme_variables, |
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.
You likely do not need this.
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.
If I comment this line out, the light specific modifications do not carry over to dark mode. I think it was working before when I had an older version of Furo but it stopped working when I updated it.
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.
Hmm... Fascinating. I'll dig into what's happening here later. I'm pretty sure this won't do the wrong thing though, so this is definitely fine to do anyway. :)
@@ -220,6 +308,7 @@ | |||
# SymPy logo on title page | |||
html_logo = '_static/sympylogo.png' | |||
latex_logo = '_static/sympylogo_big.png' | |||
html_favicon = '../_build/logo/sympy-notailtext-favicon.ico' |
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 should likely be logo/... instead? Otherwise, you won't get the logo on the first build (unless I'm misunderstanding your build process).
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.
Yeah, we compile the logo from svg to png and ico as part of the build process (the Makefile dependencies ensure it's compiled first).
If pygments gets new better-contrast themes or improves their themes, I'd be very happy to see that. I had considered writing custom pygments theme that had a better contrast story, but I decided against that to minimise how much stuff I was doing in the theme; to instead try contributing to those projects instead. I haven't actually had the time/mental space to come around to doing that though, so I welcome this! :) |
Thanks for the review @pradyunsg.
Wow that's a very subtle thing, especially with my other overrides. I guess the best thing to set this to when the sidebar has a different background color as in our case is to
I don't think we have any announcements we want to add at this point, but that's good to know for the future. We could consider adding one for the dev pages, but I still unsure if it's a good idea.
I more or less feel the same way. I didn't want to spend a whole lot of time on this, as I know it could end up being a huge rabbit hole that would ultimately produce a lot of good work, but not end up being related to the SymPy documentation. That's why I came up with a pretty hacky solution that just reuses the Ruby script I found at https://github.com/mpchadwick/pygments-high-contrast-stylesheets/blob/gh-pages/tools/make-stylesheet. It gets the job done for us, and makes the SymPy docs WCAG compatible. Ideally the whole thing would be done in Python (I'm not sure what the best library for that would be), and and be more convenient to use. For instance, one could have a function that just takes a pygments theme and automatically makes it WCAG AA compatible without requiring any manual work. Or alternately, just change the themes upstream to be AA compatible. But given that pygments specifically notates which styles have bad contrast gives me the impression that simply updating them is not a simple thing for them to do. The "sphinx" style is actually part of Sphinx, so updating that would be a separate discussion (but again, likely not a simple proposition, as it would affect a lot of projects, including docs.python.org itself). So for now I'm going to leave this as an open project for someone else to implement, but if you or anyone else ever does, please let me know. |
The Lighthouse report reveals an accessibility contrast issue with class etc. green names against a gray background, whether the green text is regular or bold: That situation seems to be the only contrast issue; the code blocks are fine. |
It's only used in two places, the left sidebar hover, and the right sidebar active page. This color was taken from the SymPy logo, and modified slightly to have a 4.5 contrast ratio. Unlike the previous color, this one now has good contrast, except in dark mode, where we use the old color for the black background.
- Use color-brand-content and color-brand-primary instead of redefining specific things. - Remove the gray selected header color. It creates bad color contrast. The default yellow is easier to work with. - The primary and content colors now have at least 4.5 contrast, including when the header is selected. - Remove some colors that were defined to nearly identical colors as the defaults (but which still have fine color contrast).
Apparently it actually is needed for color contrast compared to the default.
I fixed that and cleaned up some of the colors a bit (we now just have the two greens in color-brand-content and color-brand-primary). Also fixed some contrast issues that only showed when selecting a header (lighthouse does not see these unless you give a URL with an anchor). I also changed the hover color in the left sidebar because it had bad contrast (lighthouse does not look at hover colors). |
Great, thanks for going beyond even the automated Lighthouse checks to ensure that as many people as possible can use SymPy documentation. Comparing the current version (above) to the restyled version (below) demonstrates how much cleaner, more navigable, and more functional the restyled site will be: . |
If there are no further comments, I'd like to merge this. If we find something later that I've missed, we can make additional PRs to fix. It will be good to have this in the dev docs soon so that we can find any such issues before it gets released to the main "latest" documentation. Let's open new issues for the items we discussed above that were not addressed in this PR. |
Looks good to me. Let's get this in. |
Looks like I forgot to update the dependencies for Travis. Fix forthcoming. |
I opened several issues for things that were discussed here but not addressed (see the cross references above this comment). If I missed anything, please open an issue for it. |
This removes the separate colors for visited and unvisited. We now use the darker of the two in light mode and the ligher of the two in dark mode. These colors satisfy AA color contrast, both against the background and against the surrounding black/white text (see sympy#23159). This replaces the "brand-content" color in light mode since this is primarily used for links. We still use the lighter color for the API function names in light mode, so that they pop out more (dark mode already has a lighter color for API function names).
References to other Issues or PRs
This is based off of #22864, but I've updated the styling of the Furo theme. This is not at all finished yet, and we also haven't formally decided to switch to Furo yet either.
Demo site here: https://www.asmeurer.com/sympy-furo-demo/
Brief description of what is fixed or changed
I've done a lot of styling, but there are still things that can be improved. Please build the docs and play around with it. If you find something that is low contrast or the wrong color, it probably means it is something I didn't notice.
Also please try both the light and dark theme (you can click the icon at the top of the page).
I haven't changed any fonts or font sizes, except for the code examples. It's a possibility to do this, but I'd like feedback.
CC @bertiewooster
Other comments
TODOs
Release Notes