Skip to content

Try CSS variable again #545

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

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Conversation

agjohnson
Copy link
Contributor

Potentially replaces #492

humitos and others added 21 commits December 16, 2024 11:44
Use our heuristic to detect the documentation tool/theme and add specific
`--readthedocs-*` CSS variables based on that for known tools/themes.

Reference: readthedocs/readthedocs.org#11849 (comment)
This moves the inner CSS rules from inside the shadow DOM to the parent
DOM. This allows users to still set a `--readthedocs-font-size` and
similar variables.
This moves the inner CSS rules from inside the shadow DOM to the parent
DOM. This allows users to still set a `--readthedocs-font-size` and
similar variables.
Merge branch 'humitos/footer-style' into agj/footer-style
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is for illustration only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also for illustration only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And also not needed

Copy link
Member

Choose a reason for hiding this comment

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

We can probably create another HTML file to keep in the repository and show this as example 👍🏼

@agjohnson
Copy link
Contributor Author

Marking this for review so it doesn't get lost again. @humitos can you look at this and see if it makes sense to you?

There are some extra changes here that can be removed before merging, it's just for example.

Right now, with all of the changes and the defaults applied with the extra files, here is what the flyout and notification look like:

image

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I think this is a good pattern and it's going in a good direction.

Here you have my observations:

  • I'd like to write the CSS code in a CSS file
  • We should have a new HTML file showing an example that overrides the defaults
  • There are some properties that were calculated based in others and now they are fixed values
  • Some defaults are not defined (eg. font family)

Copy link
Member

Choose a reason for hiding this comment

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

We can probably create another HTML file to keep in the repository and show this as example 👍🏼

color: var(--readthedocs-flyout-current-version-color, #27ae60);
color: var(--readthedocs-flyout-current-version-color);
font-size: var(--readthedocs-flyout-header-font-size);
margin-left: 10px;
Copy link
Member

Choose a reason for hiding this comment

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

This property is new. Is this correct?

@hgoel0
Copy link

hgoel0 commented Apr 14, 2025

Thank you for the work on this. Would it be possible to also test this against the pydata theme?

I am happy to deploy to our rtd to test and provide feedback.

@flying-sheep
Copy link

flying-sheep commented Apr 22, 2025

See here for some currently hardcoded colors: #570 (comment)

Here’s an adapter for the pydata theme I’m currently using:

:root {
    --readthedocs-search-backdrop-color: var(--pst-color-shadow);
    --readthedocs-search-color: var(--pst-color-text-base);
    --readthedocs-search-content-background-color: var(--pst-color-background);
    --readthedocs-search-content-border-color: var(--pst-color-border);
    --readthedocs-search-filters-border-color: var(--pst-color-border);
    --readthedocs-search-font-family: var(--pst-font-family-base);
    --readthedocs-search-font-size: var(--pst-font-size-base);
    --readthedocs-search-footer-background-color: var(--pst-color-on-background);
    --readthedocs-search-footer-code-background-color: var(--pst-color-surface);
    --readthedocs-search-footer-code-border-color: var(--pst-color-border);
    --readthedocs-search-input-background-color: var(--pst-color-surface);
    --readthedocs-search-result-section-border-color: var(--pst-color-border);
    --readthedocs-search-result-section-color: var(--pst-color-link);
    --readthedocs-search-result-section-highlight-color: var(--pst-color-accent);
    --readthedocs-search-result-section-subheading-color: var(--pst-color-text-muted);
}

@agjohnson agjohnson requested a review from humitos April 23, 2025 16:40
@agjohnson
Copy link
Contributor Author

agjohnson commented Apr 23, 2025

See here for some currently hardcoded colors: #570 (comment)

I had skipped the search addon for this first pass, but it probably makes sense to also use these changes here. I'll replicate these variables and the new ones from your #571 in this PR 👍

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I re-review this and I think the pattern is correct 👍🏼

The last 3 bullets from my previous review are still valid: #545 (review)

@agjohnson
Copy link
Contributor Author

agjohnson commented May 1, 2025

We should have a new HTML file showing an example that overrides the defaults

You can add these variables on the existing example test page as a rule on html. I'm not sure there's a strong purpose yet for a development page like this yet as there isn't any way to validate what you are supposed to see with a page like that.

We can unit test some of these styles with web-test-runner though, and should probably eventually use a visual diffing tool like Happo to just automate this.

There are some properties that were calculated based in others and now they are fixed values

What are you referring to here? There are a few places that a complex calc(var(...) * 1.125) was replaced by a much simpler 1.125em, is this what you're referring to?

Some defaults are not defined (eg. font family)

This was already resolved.

@agjohnson
Copy link
Contributor Author

agjohnson commented May 1, 2025

I am still poking this as the search addon is quite chaotic on font-sizes. It overuses font-size and uses all sorts of different units are used for sizing and positioning: rem, em, px, and small. So the font size is customizable but it renders inconsistently:

image

image

@humitos
Copy link
Member

humitos commented May 6, 2025

What are you referring to here? There are a few places that a complex calc(var(...) * 1.125) was replaced by a much simpler 1.125em, is this what you're referring to?

Yes, I mentioned this at #545 (comment)

It seems you already replied to that and I've missed.

agjohnson added 2 commits May 7, 2025 09:52
Just a basic font-size and color example, we can build on these as we go
@agjohnson
Copy link
Contributor Author

I wrote up a quick example of what CSS style testing would look like in 4b683a3. I didn't do this across our addons or customization points yet, but this would be easy to expand on.

I'm going to push off fixing issues with font sizes in search to a later issue. The font size issue is a current bug with search already:

image

So I think we can continue with this from here. Follow up work to this issue can be:

  • Test this in production more
  • Tests: improve coverage on CSS customization unit tests
  • Search: fix inconsistent and static font size usage
  • Better addons documentation #577 to document customization and defaults better

--readthedocs-search-footer-code-border-color: #e1e4e5;
--readthedocs-search-input-background-color: #eaeaea;

/* `--readthedocs-search-result-section-highlight-color` is deprecated but
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result-section prefix confused me, as these rules target not the "section" -- ie the surrounding element around results -- but are instead for the individual results. So I think these should all be search-result-background-color not search-result-section-background-color, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I don't remember exactly how this works, but I remember that a section can have multiple results -- if that helps 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a div.results that has all of the individual page .hit elements and each .hit can have multiple nested results. But the styles in these variables aren't affecting div.results, what I assumed section referred to. It's ambiguous enough it's worth removing either way.

@agjohnson agjohnson requested a review from humitos May 7, 2025 17:09
@agjohnson
Copy link
Contributor Author

@humitos I'd take one last look at the change here. Notable changes at this point:

  • Slight CSS unit test coverage. Anything else to cover immediately?
  • Not a big deal, but I noticed that tests do need to load the default style sheets manually now. Without loading the defaults, unit tests load partially unstyled elements.
  • Some CSS variables have changed slightly

@humitos
Copy link
Member

humitos commented May 12, 2025

We can unit test some of these styles with web-test-runner though, and should probably eventually use a visual diffing tool like Happo to just automate this.

Yeah, no, I'm not talking about something complex like this. I was thinking of just having a page using the CSS variables with noticeable changes (very big flyout/search/etc) just to quickly see they have effect.

@humitos
Copy link
Member

humitos commented May 12, 2025

I am still poking this as the search addon is quite chaotic on font-sizes. It overuses font-size and uses all sorts of different units are used for sizing and positioning: rem, em, px, and small.

This was quickly copied from the old Sphinx extension without thinking too much about it or doing any modification. Feel free to re-structure it with a more modern pattern.

--readthedocs-search-footer-code-border-color: #e1e4e5;
--readthedocs-search-input-background-color: #eaeaea;

/* `--readthedocs-search-result-section-highlight-color` is deprecated but
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember exactly how this works, but I remember that a section can have multiple results -- if that helps 😄

const element = document.querySelector("readthedocs-flyout");
await elementUpdated(element);

// Addon default of 80% of global --readthedocs-font-size, default 16px
Copy link
Member

Choose a reason for hiding this comment

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

Where this "default 16px" comes from? We are defining these values in em, how they are converted into px at start up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

4 participants