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

Allow paragraph elements to inherit color #1762

Merged
merged 2 commits into from
May 8, 2024

Conversation

gabalafou
Copy link
Collaborator

@gabalafou gabalafou commented Apr 9, 2024

As #741 describes, we make notebook cell outputs "white-ish" even when our theme is in dark mode because dark mode isn't well supported by the tools that render notebook cell outputs to HTML.

However, our theme also sets the font color of p elements to a CSS variable that changes depending on whether the theme is set to light or dark mode.

These two things combined to create a bug, by layering the following colors on top of each other:

  1. dark site background
  2. light cell output background
  3. light paragraph text

This made some of the text on notebook cell outputs invisible in dark mode. Here is a pair of before and after screenshots:

before after
pydata-sphinx-theme readthedocs io_en_latest_examples_pydata html (1) pydata-sphinx-theme readthedocs io_en_latest_examples_pydata html

Notice the text in the lower left that reads "100 rows × 26 columns"? That text is contained in a p-tag.

This PR removes the CSS color rule on the p selector so that the paragraph can inherit its color from the nearest parent element that has it set.

I think this change makes sense because both body { color } and p { color } are (before this PR) set to the same CSS variable, var(--pst-color-text-base). In the past, the paragraph color was set to a different CSS color variable, which was probably done to allow theme consumers to style copy color separate from the color of other text on the site, but since that distinction has since been removed, I don't think it makes sense to set p { color } explicitly. Just let it inherit.

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

As long as this doesn't create unexpected side effects then this feels like a reasonable idea to me!

@gabalafou gabalafou marked this pull request as draft April 9, 2024 13:26
@gabalafou
Copy link
Collaborator Author

Definitely risk for unintended side effects, for example:

screenshot showing PyData Theme logo text is dark on dark

@gabalafou gabalafou marked this pull request as ready for review April 29, 2024 09:26
@gabalafou
Copy link
Collaborator Author

Okay, I think this is ready for merge now. I checked lots of pages in both light and dark mode, hovered and focused on lots of elements, couldn't find any places where the styles looked wrong.

@trallard trallard added the tag: CSS CSS and SCSS related issues label May 8, 2024
@trallard trallard merged commit 4fd33b2 into pydata:main May 8, 2024
16 of 17 checks passed
@gabalafou gabalafou deleted the p-inherit-color branch May 8, 2024 10:57
ivanov pushed a commit to ivanov/pydata-sphinx-theme that referenced this pull request Jun 5, 2024
* Allow paragraph elements to inherit color

* Set paragraph color where necessary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: CSS CSS and SCSS related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants