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

ENH - Scalable colour system for theme #1174

Merged
merged 77 commits into from
Jun 20, 2023
Merged

ENH - Scalable colour system for theme #1174

merged 77 commits into from
Jun 20, 2023

Conversation

trallard
Copy link
Collaborator

@trallard trallard commented Feb 13, 2023

Closes #1079
Closes #1052

As discussed in gh-1079 this PR will focus on adopting the new colour scheme and not any substantial style changes (these will come in a separate PR). Though I might need to adjust some styles/variables as part of this work, I will do my best to keep you all aligned and update the docs as needed.


🏷️ Edited: I am taking this out of WIP to encourage reviews, comments and questions.

Things I am still not super happy with or need work on (I will check the items as I work on them):

  • Scrollbars - I am not super happy with the current colours; they are either too dark or too light and draw a lot of attention
  • Buttons - these inherit from sphinx design - I have changed the formula to decide on the text colour (using WCAG luminance and contrast ratio formulas but for some reason the mixins in _sphinx_design.scss are giving me trouble so will debug here)
  • Badges - same issue as buttons
  • Buttons - fix hover colour (separate PR for buttons)
  • Fix dropdown colours (sphinx design - little tweaks)
  • Little tweaks needed on the admonition colour mixin
  • Shadows improvements (nothing major, simple tweaks here and there)
  • Adjust banner link colours - light theme only
  • Documentation:
    • Update accessibility section (user)
    • Update accessibility section (developer)
    • Update theming and customisation sections

Sans those little nags and whatever we encounter during the review, this seems to be in a decent place to start reviewing and hopefully merge soonish 🚀

Additional notes:

  • Right now, the announcement bars use the info colour for the bg: in the proposal Figma file I had used primary/secondary/accent for these. I changed the colour to secondary in this PR, so I have the following questions
    • Do we want to keep info as the default colour? I am neutral about this
    • Shall we have 2-3 slight variants so folks can choose the colour from, let's say, info, primary, secondary, accent?

For quick checks here is the link to the Read the docs PR preview

@trallard trallard marked this pull request as draft February 13, 2023 12:18
Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

I know this is WIP but I was curious 🙂 just two little comments of things I noticed along the way

@trallard
Copy link
Collaborator Author

Thanks - this is due to change plenty as I work towards the final bits. But an early review is always welcome.

@trallard
Copy link
Collaborator Author

trallard commented Feb 19, 2023

🤔 As I have been working on the theme today, I started getting a weird error while trying to use nox -s docs-live while it was working well before, now it fails with the following error:

File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/jupyter_cache/cache/main.py", line 300, in get_cache_bundle
    raise KeyError(f"Notebook file does not exist for cache record PK: {pk}")
KeyError: 'Notebook file does not exist for cache record PK: 1'

Exception occurred:
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/jupyter_cache/cache/main.py", line 300, in get_cache_bundle
    raise KeyError(f"Notebook file does not exist for cache record PK: {pk}")
KeyError: 'Notebook file does not exist for cache record PK: 1'
The traceback has been saved in /var/folders/6p/062gm41556s5p63x1555cjzw0000gn/T/sphinx-err-i8usg03k.log, if you want to report the issue to the developers.

About my setup:

Sphinx version: 5.3.0
# Python version: 3.9.16 (CPython)
# Docutils version: 0.19 
# Jinja2 version: 3.1.2

Extended log-in here if needed

# Sphinx version: 5.3.0
# Python version: 3.9.16 (CPython)
# Docutils version: 0.19 
# Jinja2 version: 3.1.2
# Last messages:
#   reading sources... [ 50%] examples/kitchen-sink/really-long
#   reading sources... [ 51%] examples/kitchen-sink/structure
#   reading sources... [ 52%] examples/kitchen-sink/tables
#   reading sources... [ 53%] examples/kitchen-sink/typography
#   reading sources... [ 54%] examples/mult_headers
#   reading sources... [ 56%] examples/no-sidebar
#   reading sources... [ 57%] examples/persistent-search-field
#   reading sources... [ 58%] examples/pydata
#   /Users/tania/Documents/github/patches/pydata-sphinx-theme/docs/examples/pydata.md: Using cached notebook: ID=1 [mystnb]
#   sphinx-sitemap: No pages generated for sitemap.xml
# Loaded extensions:
#   sphinx.ext.mathjax (5.3.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/ext/mathjax.py
#   sphinxcontrib.applehelp (1.0.4) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinxcontrib/applehelp/__init__.py
#   sphinxcontrib.devhelp (1.0.2) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinxcontrib/devhelp/__init__.py
#   sphinxcontrib.htmlhelp (2.0.1) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinxcontrib/htmlhelp/__init__.py
#   sphinxcontrib.serializinghtml (1.1.5) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinxcontrib/serializinghtml/__init__.py
#   sphinxcontrib.qthelp (1.0.3) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinxcontrib/qthelp/__init__.py
#   alabaster (0.7.13) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/alabaster/__init__.py
#   sphinx.ext.autodoc.preserve_defaults (5.3.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/ext/autodoc/preserve_defaults.py
#   sphinx.ext.autodoc.type_comment (5.3.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/ext/autodoc/type_comment.py
#   sphinx.ext.autodoc.typehints (5.3.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/ext/autodoc/typehints.py
#   sphinx.ext.autodoc (5.3.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/ext/autodoc/__init__.py
#   sphinx.ext.autosummary (5.3.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/ext/autosummary/__init__.py
#   sphinx.ext.todo (5.3.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/ext/todo.py
#   sphinx.ext.viewcode (5.3.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/ext/viewcode.py
#   sphinxext.rediraffe (unknown version) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinxext/rediraffe.py
#   sphinx_design (0.3.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx_design/__init__.py
#   sphinx_copybutton (0.5.1) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx_copybutton/__init__.py
#   ablog (0.11.0rc2) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/ablog/__init__.py
#   jupyter_sphinx (0.4.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/jupyter_sphinx/__init__.py
#   matplotlib.sphinxext.plot_directive (3.7.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/matplotlib/sphinxext/plot_directive.py
#   myst_nb (0.17.1) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/myst_nb/__init__.py
#   sphinxcontrib.youtube (unknown version) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinxcontrib/youtube/__init__.py
#   numpydoc (1.5.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/numpydoc/__init__.py
#   sphinx_togglebutton (0.3.2) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx_togglebutton/__init__.py
#   sphinx_sitemap (2.5.0) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx_sitemap/__init__.py
#   pydata_sphinx_theme (unknown version) from /Users/tania/Documents/github/patches/pydata-sphinx-theme/src/pydata_sphinx_theme/__init__.py
Traceback (most recent call last):
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/cmd/build.py", line 281, in build_main
    app.build(args.force_all, args.filenames)
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/application.py", line 343, in build
    self.builder.build_all()
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/builders/__init__.py", line 269, in build_all
    self.build(None, summary=__('all source files'), method='all')
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/builders/__init__.py", line 326, in build
    updated_docnames = set(self.read())
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/builders/__init__.py", line 433, in read
    self._read_serial(docnames)
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/builders/__init__.py", line 454, in _read_serial
    self.read_doc(docname)
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/builders/__init__.py", line 510, in read_doc
    publisher.publish()
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/docutils/core.py", line 224, in publish
    self.document = self.reader.read(self.source, self.parser,
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/sphinx/io.py", line 104, in read
    self.parse()
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/docutils/readers/__init__.py", line 76, in parse
    self.parser.parse(self.input, document)
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/myst_nb/sphinx_.py", line 150, in parse
    with create_client(
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/myst_nb/core/execute/base.py", line 83, in __enter__
    self.start_client()
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/myst_nb/core/execute/cache.py", line 36, in start_client
    _, self._notebook = cache.merge_match_into_notebook(self.notebook)
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/jupyter_cache/cache/main.py", line 357, in merge_match_into_notebook
    cache_nb = self.get_cache_bundle(pk).nb
  File "/Users/tania/Documents/github/patches/pydata-sphinx-theme/.nox/docs-live/lib/python3.9/site-packages/jupyter_cache/cache/main.py", line 300, in get_cache_bundle
    raise KeyError(f"Notebook file does not exist for cache record PK: {pk}")
KeyError: 'Notebook file does not exist for cache record PK: 1'

I have removed my .nox directory a couple of times and had no luck - I might try to manually setup my env tomorrow, as it would be best if I could build and serve the docs locally to finish the PR.
The docs, however, seem to be building in Read the docs see https://pydata-sphinx-theme--1174.org.readthedocs.build/en/1174/

If anyone has any pointers here it would be grand

@trallard trallard self-assigned this Feb 19, 2023
@drammock
Copy link
Collaborator

Notebook file does not exist for cache record PK

This is a new one, never seen it before. It's coming from jupyter-cache via Myst-NB, which means @choldgraf is probably the most knowledgable here. Are you still stuck there @trallard ?

@trallard
Copy link
Collaborator Author

I resourced to start from scratch - new env, clone repo again and it's the only way I managed to get rid of this.

I am not sure how this happened tbh as it only appeared after I merged main back into my branch.

@choldgraf
Copy link
Collaborator

Re: the cache, I think you need to delete the jupyter_cache folder in the build directory, that's where all the cached notebooks go. Then it should review from scratch

@trallard
Copy link
Collaborator Author

I tried that too but the only workaround was starting from scratch.

@trallard
Copy link
Collaborator Author

As I am adding the finishing touches and updates to docs I realised we'd need to update the screenshot in the Readme. Does anyone know how to create this type of half and half images?

@12rambau
Copy link
Collaborator

12rambau commented Jun 16, 2023

I did them with powerpoint. take 2 screenshots of the same page in each theme and cut one of them with a triangle then combine and save as image. The shadow is a byproduct of Mac screeenshots but I'm not sure it's necessary.

I'm sure it's not the more elegant way to proceed but that would have take me more time to find another way.

@trallard
Copy link
Collaborator Author

trallard commented Jun 16, 2023

@drammock, thanks for the feedback on the admonitions, I fixed the colours on the light theme, and I agree these look much in line with the dark theme now

pyData Sphinx theme admonitions page

Also, I replaced the inline code borders in the dark theme for a less bright shade of gray so hopefully, this will be less disruptive/jarring.

pydata sphinx theme pandas example

As for the highlight target colour - as @12rambau mentioned, it is pretty hard to get colours with good contrast against all possible text colours in the theme. I tried many combinations, and this is the best I could get to:

Read_the_Docs_functionality_—_PyData_Theme_0_13_4dev0_documentation

Read_the_Docs_functionality_—_PyData_Theme_0_13_4dev0_documentation

The target colour #675c04 for the dark theme meets WCAG AA contrast against links, text (4.5:1), and background (3:1)
the one on the light theme is way trickier to get the right contrast against background and text colours as these are opposite (white vs. almost black), so the best I could do was set this to #f3cf95 which is also slightly shifted towards red vs. the dark target theme (IMHO not a big deal). But overall, these have better contrast and legibility than the ones we had.

Note that in a separate PR I will add underline to the links as these are needed to meet WCAG (not solely rely on colour) so that will also help with any contrast issues against underline/search highlight.

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

The changes since my last review look great! Thanks @trallard. This time I did a thorough code review instead of just reviewing the built docs. Most comments/suggestions are minor tweaks to wording in/around the parts of the docs that are changed in this PR, but there are a few substantive questions in there about the CSS/SASS too. LMK if anything is unclear; I think we are 99% there.

.jupyterlite.doit.db Outdated Show resolved Hide resolved
docs/community/topics/bootstrap.rst Outdated Show resolved Hide resolved
docs/community/practices/versions.md Outdated Show resolved Hide resolved
docs/user_guide/accessibility.rst Outdated Show resolved Hide resolved
docs/user_guide/accessibility.rst Outdated Show resolved Hide resolved
@choldgraf
Copy link
Collaborator

Just took a quick look, I am +1 on merging. I think it's a clear improvement, and we will get more feedback from ourselves and others once we put it in front of people so I think it's best to merge and iterate rather than trying to get it perfect this time. If y'all want to finish a few things first though that's fine too.

@drammock
Copy link
Collaborator

hooray!!! in it goes, thanks a million @trallard this is a big big improvement that I'm really pleased with.

@drammock drammock merged commit 40a3032 into pydata:main Jun 20, 2023
@choldgraf
Copy link
Collaborator

Woohoo! 🎉

@trallard
Copy link
Collaborator Author

🎉 Thank you folks it was a long and complex PR, but y'all were more than supportive and open throughout all these months!

@trallard trallard deleted the trallard/color-system branch June 20, 2023 15:09
@12rambau
Copy link
Collaborator

should we make a feature release so that people give us feedback ?

@trallard
Copy link
Collaborator Author

It might be helpful, but could we wait until #1353 is merged? With those changes we fix a lot of accessibility issues cropping in our tests

@12rambau
Copy link
Collaborator

I added the block release tag to make sure I don't forget

@Gouvernathor
Copy link

Gouvernathor commented Nov 9, 2023

So, apparently this is the place where the dark theme's background color was changed to bluish 😥
I don't see any discussion on anything blue, and discussions related to the background either mention the light background or pertain to how the color of other things relates to the background color having already been changed.

I find the former, very dark grey color was much better in the style and impression it gave out to the rest of the page, and that this particular change is quite bad.
I also though the same about the link colors that were changed (maybe not in this PR), until I learned it was related to accessibility, which makes it acceptable.
However, in the case of the dark theme's background, I don't see it. Since the font color is white, the contrast between the font and te background decreases when you change it from almost pitch-black to bluish, so how can that be supported by an accessibility rationale ?

I guess it may need an issue to start repealing this particular change (I'm not opposed to all the changes in this PR of course), but I would like to learn the rationale for it to understand the context first.

@12rambau
Copy link
Collaborator

12rambau commented Nov 9, 2023

As mentioned in the first comment of this fantastic PR (thank you again @trallard for your commitment) these modifications were done after a long conversation in #1079 I don't remember all the threads but no color was changed without a good justification from the a11y experts.
From a personal POV although I wasn't a fan of the teal as primary color I love the final render.

Note that if you want, you can still revert the color to what they were in your custom.css.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: accessibility Issues related to accessibility issues or efforts
Projects
None yet
5 participants