[ENH]: Hard-code dynamic Tailwind util classes (improve ToC styling)
Description of changes
Fixes #4129
- The doc-site codebase uses Tailwind CSS utility classes (ex.
.pl-2,.pl-3=>padding-left: 0.5rem;,padding-left: 0.75rem;, etc.), which provide short-hand, computed classes from Tailwind (very handy). - Tailwind ships an optimized production bundle by doing static analysis and only including style declarations for classes (including utility classes) that it finds in the code.
- So, trying to use a utility class that is dynamically constructed at runtime via props or something doesn't (usually) work, because if you write, e.g.,
.pl-${props.item.number}, it doesn't know which.pl-<VALUE>classes it has to include. (Caveat: Sometimes you get away with it because elsewhere in the code someone wrote.pl-4so, by accident, if your dynamic class is.pl-4at runtime, then it will find the style declarations for it, and all is well.) - Long story short, you can't rely on dynamic utility classes (see Issue for (even) more long-winded explanation).
- This appears a few times in the codebase, and probably it's not a severe issue. The case that brought it to my attention is the Table of Contents, where I noticed that sub-sections (h2's) are indented more than top sections (h1's), but that h3's de-indent and are at the same level as h1. Example:
"Getting Started" and "Contributing Code and Ideas" have class .pl-2 (they're h2's), and "Pull Requests", "CIPs", and "Discord", have class .pl-3 (they're h3's), but they are not indented further under the h2's. So the hierarchy of header levels is not mirrored in the table of contents indenting.
The reason is that .pl-2 is hard-coded elsewhere in the codebase, so the style declaration gets bundled and shipped, but .pl-3 is not... so when the style .pl-3 gets computed, it doesn't match any style declaration, and it doesn't compute the padding-left.
Fix
Instead of refactoring out all of the dynamic utility classes, or coming up with an overly-clever solution to make it work, this PR is a patch to handle .pl-X classes to make the table of contents work as intended. You can easily figure out that .pl-x resolves to padding-left: ${0.25 * x}rem;
So to fix the ToC, assuming a generous potential heading-hierarchy of 10 levels, I just hard-coded .pl-{0..10} into globals.css.
The Sequel
When I initially did this, it didn't really look that interesting (i.e., the indenting is not that noticeable):
THIS is because the h1 items were getting .pl-1, but .pl-1 itself was not defined, so they had padding-left: 0. So the padding-left went from 0rem, to 0.5rem (pretty noticeable), but after declaring .pl-1 in the "expected" way, it went from 0.25rem to 0.5rem, then to 0.75rem (not that noticeable).
SO, in order to make this all come together and still keep the relative differences between the header levels' padding-left the same as before, while adding support for further levels of indenting, I decided to set the Table of Contents .pl-<LEVEL> values to be (level - 1) * 2, as in:
- h1 =>
.pl-0 - h2 =>
.pl-2 - h3 =>
.pl-4 - etc.
The result is that h1's and h2's are indented the same way as before (0rem, 0.5rem), and subsequent header levels are indented with the same delta as previously between h1's and h2's (i.e. 0.5rem more). The result is this:
Which is more noticeable and overall looks better, IMO, and doesn't involve re-defining any of the .pl- utility values themselves, so they can still be relied on to work the same as the Tailwind defaults.
Another cool example. (You're still reading this?)
The reference/js/collection page.
BEFORE:
Class is no indent, Methods is indented, but then individual methods are de-indented (e.g. add is de-intended relative to Methods.
AFTER:
Indenting of Class > indenting of Methods > individual method (e.g. "add") > "Parameters"
Test plan
How are these changes tested?
Run the app locally, look at the classes assigned to the different ToC items, compare to their corresponding header-levels in the main body, look at their indentings, see that all .pl- utility classes actually have style definitions.
Ran the build, no failures.
- [ ] Tests pass locally with
pytestfor python,yarn testfor js,cargo testfor rust
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?
Reviewer Checklist
Please leverage this checklist to ensure your code review is thorough before approving
Testing, Bugs, Errors, Logs, Documentation
- [ ] Can you think of any use case in which the code does not behave as intended? Have they been tested?
- [ ] Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
- [ ] If appropriate, are there adequate property based tests?
- [ ] If appropriate, are there adequate unit tests?
- [ ] Should any logging, debugging, tracing information be added or removed?
- [ ] Are error messages user-friendly?
- [ ] Have all documentation changes needed been made?
- [ ] Have all non-obvious changes been commented?
System Compatibility
- [ ] Are there any potential impacts on other parts of the system or backward compatibility?
- [ ] Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?
Quality
- [ ] Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)
@jeffchuber just tagging you bcz I think you've been on some of the front-end PR's lately, hope that's all right
@jeffchuber bump
Hey @hesreallyhim, we don't want to override the Tailwind styles. I prepared an example with a simple workaround here if you want to take a look: https://github.com/chroma-core/chroma/pull/4493.
You'll see that another bug exists when sections have the same name (and thus ID), we will not scroll the page correctly. Let me know if you're up for the challenge!
@itaismith yeah you're correct that's non-optimal to override them. i'll have to spin up your branch and see what you're trying to achieve, but i do think my solution is roughly correct if the scope is just the ToC (and if you want it to look the way it does in the screenshot of my branch). Apparently Tailwind reads source files as plain text and doesn't even parse them, so even a line like:
const dummy = "pl-0 pl-1 pl-2 pl-3 pl-4 pl-5 pl-6..."
will cause Tailwind to include those classes if i understand correctly. So the point being - the indentation logic was already implemented, it's just the class declarations that were missing. So a minimal bug fix would be to just include those classes in the simplest possible way. So I think probably you are doing something a little wider scope, or doing some refactoring (obv all good), or the solution I propose is kind of relying on weird knowledge about how tailwind works, but anyway that's another alternative, and regardless it's interesting to learn that Tailwind doesn't parse source code for this aspect. I'll try to peruse your PR, cheers!