chroma-core
[DOC]: Illegal use of dynamic Tailwind utility classes in docs
Description
The docs use Tailwind utility classes for styling a lot ("h-5" (height), "pr-1" (padding-right), etc.), which is really convenient and nice. The problem is, because Tailwind produces optimized production code by only bundling the classes that are consumed in the components, which it determines based on static analysis, writing class-names that are composed from run-time variables, like React component props, is prone to failure. For instance, bg-${props.item.color} may not work, because Tailwind will not know what bg-<color> classes it needs to ship with the code, and so it turns out the class .bg-pink has no styles declared. Sometimes you can get away with it, because somewhere else in the codebase, bg-pink is used explicitly, and therefore gets bundled with the code, and so if your component gets .bg-pink at runtime, it will luckily find the classname and get the correct style.
For the most part, this is probably a pretty insignificant anti-pattern, but the reason I'm bringing it up is because of the Table of Contents in the Docs - I noticed that they don't do recursive indentation of sub-classes/methods/etc. For instance:
The headers on the page are H1, H2, H2, and then H3, H3, H3. But as you can see, the H3's (Pull Requests, CIPs, and Discord) are de-indented, whereas a natural assumption would be that each sub-header gets indented more. Indeed, that appears to be the intention of the code, because the classes for the corresponding names in the ToC include .pl-1, (padding-left-1), .pl-2, and .pl-3.
The problem is these padding utility classes are defined with template strings based off props, where each item is dynamically assigned a "level" property through some code, and then the class is something like pl-${level}. Because, coincidentally, .pl-1 and .pl-2 are used elsewhere in the codebase in a static fashion, their styles are included in the production bundle - but .pl-3 isn't. So it does nothing. This is why the ### H3's have no padding/indentation.
For reference:
- https://tailwindcss.com/docs/detecting-classes-in-source-files#dynamic-class-names
- https://github.com/chroma-core/chroma/blob/6586c1c28920210731c51801178482aa6c72a7d8/docs/docs.trychroma.com/components/table-of-contents.tsx#L22
toc.map((item) => ( <div key={item.id} className={
mt-1 **pl-${item.level * 1}**}>
Solution
There's probably some clever solutions to achieve dynamic utility classes, and there are a few other places in the code where this anti-pattern appears, but a really simple way to fix this is just to add .pl-1/2/3/4/5 to the globals.css in order to fix the ToC and call it a day. The only question is whether the maintainers actually would like to see progressive indentation for the Table of Contents. If so, I'd be happy to put together a PR that fixes this issue, so that, e.g. the last three items on the ToC in the screenshot above are actually indented further, and the hierarchical structure of the page is reflected in the ToC. Since the upper bound on heading levels is pretty small, we can just ship them all and it's almost no cost.
Although it's clearly been written in such a way that it's supposed to have recursive indentation, I'd just like an OK from a maintainer that this would be a minor cosmetic improvement, and I'll go ahead and submit a fix.
Either way, we should stop writing dynamic utility classes, because it's not supported by Tailwind.
@hesreallyhim do you want to give that a shot? really great write-up!
Yeah no problem, it should be a quick patch if we just want to handle the table of contents issue I noted. I'll just add "pl-{1..5}" to the global styles
Hi @nikd10 , I opened a PR to fix it, but it didn't get merged, you should check out the thread here for more info: https://github.com/chroma-core/chroma/pull/4307 and maybe follow up with the other participant if you want more info, I'm not sure what the status is. A brief glance at the website suggests to me that the ToC issue is not resolved, so might be fair game.