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

Add styles based on the documentation tool #473

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

Conversation

humitos
Copy link
Member

@humitos humitos commented Dec 16, 2024

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)

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)
@humitos humitos requested a review from a team as a code owner December 16, 2024 10:46
@humitos humitos requested a review from agjohnson December 16, 2024 10:46
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

I believe there is a better pattern here for dynamic styles. The Lit documentation probably suggests using classMap on the element, which could be another option. We've talked about avoiding classes for styling though, so I mostly lean towards a dynamic styles getter if it works easily.

src/flyout.js Outdated
document.adoptedStyleSheets.push(styleMkDocsMaterial);
} else if (doctool == SPHINX && docTool.isSphinxFuroLikeTheme()) {
document.adoptedStyleSheets.push(styleSphinxFuro);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like an odd pattern, especially applying rules to :root from a shadow dom element. Normally, these rules would be applied to only this single element, not as a global style rule.

This is maybe a place where a styles getter can be used instead, especially if none of the docTool.* functions require a bound this referencing the current element.

@humitos
Copy link
Member Author

humitos commented Dec 17, 2024

I believe there is a better pattern here for dynamic styles. The Lit documentation probably suggests using classMap on the element, which could be another option

Yes. Is there are way to detect if the --readthedocs-* variables were defined by the user from JavaScript? The pattern that I want to implement here is:

  1. Check if the user defined --readthedocs-* CSS variables
  2. If those were defined, do nothing
  3. If not, add a mkdocs-material CSS class to the element that redefines --readthedocs-*

Would that make sense?

I mostly lean towards a dynamic styles getter if it works easily.

I'm not sure to understand what this means. Can you expand o this?

@humitos
Copy link
Member Author

humitos commented Dec 17, 2024

I pushed a commit to what I understood it could be a better pattern for this. Let me know what you think.

@agjohnson
Copy link
Contributor

I'm not sure to understand what this means. Can you expand o this?

Using a getter for styles property allows for conditional styles without adding class names.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get#using_getters_in_classes

class SomeElement extends LitElemeent {
  static get styles {
    if (...) {
      return css`...`;
    }
    return css`...`;
  }
}

Would that make sense?

So, there is getComputedStyle: https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle

But it feels like better design to make this explicit and rely on existing CSS functionality for this. If we keep our rules low specificity, then user added rules can very easily be higher specificity -- and our styles are then overridden.

If not, add a mkdocs-material CSS class to the element that redefines --readthedocs-*

Using class names is a separate pattern, but also possible.

In general, a pattern I like is to (re)use element properties for CSS rules instead of classes. Element properties have specific purpose and update the Lit element properties where class names don't do anything but affect CSS styles. That is:

// The `tool` property affects the element reactive properties. This rule is specificity 0 1 1
readthedocsflyout[tool="docusaurus"] { ... }
// The CSS class is used only for styles, it doesn't affect the properties on the Lit element. Specificity 0 1 1
readthedocsflyout.tool-docusaurus { ... }

We should keep specificity as low as possible either way.


One wrench I'll give you though is that this is still just for readthedocs-flyout, and we probably don't want to have each element independently guessing the tool and applying styles. I'm guessing your initial attempt was closer to what you wanted. What I pointed out as the problem applying styles to html/:root from inside FlyoutElement is that this affects styles of other elements, and likewise doesn't apply these styles if the user has the flyout disabled.

So I think the styles you applied originally might be closer to what we want, we just shouldn't do this from inside a single element. This is something we should do a more global level.

I might still suggest a class/attribute selector approach here though, which leverages CSS functionality instead of hiding this logic from user in JS.

:root[data-readthedocs-tool="docusaurus"] { ... }
:root[data-readthedocs-tool="sphinx"] { ... }

This gives a clear attribute users can remove and we can avoid applying an attribute if a user already specifies an attribute or something like data-readthedocs-tool="manual".

@@ -20,6 +26,7 @@ export class FlyoutElement extends LitElement {

static properties = {
config: { state: true },
classes: { state: true, type: Object },
Copy link
Contributor

@agjohnson agjohnson Dec 17, 2024

Choose a reason for hiding this comment

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

Element already has a Element.classList, classes is mostly reproducing that native function. However my note on using properties over class names is most applicable here.

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

Successfully merging this pull request may close these issues.

2 participants