Skip to content

Conversation

@david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Mar 5, 2024

Changes

  • Moved all CSS imports that took place TS files into the main index.css file so we can control the order
  • Moved all CSS files into app/ui/styles — this is debatable, I guess they could still live next to their components, but meh
  • Clean up dialog styles
  • Resolve TODO comments on dialog styles

Why

postcss-import is a PostCSS plugin that takes all your CSS @import statements and inlines them into a single file. If you're just working with CSS files importing other CSS files, the order should be straightforward: depth-first traversal of the import tree. Vite also, somewhat magically, inlines CSS files imported into TypesSript files. We were importing CSS that way sometimes so the CSS would be right next to the relevant code, e.g., importing the button CSS in Button.tsx. The problem, as far as I can tell, is that with imports into TS files, the order that things get inlined into the final CSS output is not well-defined, or at least not easy to predict.

In #1978, I shuffled a bunch of files around and moved the import of the main CSS bundle from the now-deceased UI barrel file all the way up to main.tsx, and this seems to have caused the order that CSS imports were inlined into the bundle to change, which broke button styling by failing to put it after the base Tailwind styles it is meant to override.

Bad

image

Good

image

The selectors, being single classes, were not more specific than the base Tailwind styles. Make them more specific could be one approach to fixing this.

At this point the Tailwind expert should ask: isn't this what @layer is for? Yes, this is what @layer is for. Unfortunately, when you're mixing imports into CSS and imports into TSX, postcss-import doesn't see to be executing correctly, so when it see @layer in a the CSS file imported into TS, it doesn't seem to know that the necessary @tailwind is present over in another CSS file.

x Build failed in 1.22s
error during build:
CssSyntaxError: [vite:css] [postcss] /Users/david/oxide/console/app/ui/lib/button.css:9:1: `@layer components` is used but no matching `@tailwind components` directive is present.
file: /Users/david/oxide/console/app/ui/lib/button.css:9:0
    at Input.error (/Users/david/oxide/console/node_modules/postcss/lib/input.js:106:16)
    at AtRule.error (/Users/david/oxide/console/node_modules/postcss/lib/node.js:115:32)
    at normalizeTailwindDirectives (/Users/david/oxide/console/node_modules/tailwindcss/lib/lib/normalizeTailwindDirectives.js:72:32)
    at /Users/david/oxide/console/node_modules/tailwindcss/lib/processTailwindFeatures.js:30:98
    at plugins (/Users/david/oxide/console/node_modules/tailwindcss/lib/plugin.js:38:69)
    at LazyResult.runOnRoot (/Users/david/oxide/console/node_modules/postcss/lib/lazy-result.js:329:16)
    at LazyResult.runAsync (/Users/david/oxide/console/node_modules/postcss/lib/lazy-result.js:258:26)
    at async compileCSS (file:///Users/david/oxide/console/node_modules/vite/dist/node/chunks/dep-G-px366b.js:32012:25)
    at async Object.transform (file:///Users/david/oxide/console/node_modules/vite/dist/node/chunks/dep-G-px366b.js:31327:56)
    at async transform (file:///Users/david/oxide/console/node_modules/rollup/dist/es/shared/node-entry.js:17501:16)

It does not give this error on @layer if the same CSS file is imported directly into index.css instead, but once we're doing it all in CSS, we control the order anyway, so we don't need @layer.

I think the fact that this worked before was basically luck. Looking at the compiled CSS output from a "good" deploy, the base TW stuff is still not at the beginning of the file, it just happens to be somewhere in the middle and the button CSS goes after it. It's good that we turned up this problem now when it wasn't particularly inconvenient and we have time to fix it.

@vercel
Copy link

vercel bot commented Mar 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Mar 6, 2024 3:53pm

background: white;
padding: 2rem;
outline: none;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had copied this stuff in from Reach dialog when we switched to Radix, even though almost all of it was being overridden.

https://github.com/reach/reach-ui/blob/43f450db7bcb25a743121fe31355f2294065a049/packages/dialog/styles.css#L18-L24

className={({ isActive }) =>
cn(linkStyles, {
'!bg-accent-secondary hover:!bg-accent-secondary-hover svg:!text-accent-tertiary':
'text-accent !bg-accent-secondary hover:!bg-accent-secondary-hover svg:!text-accent-tertiary':
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I accidentally deleted this while testing HMR performance after removing barrel files.

@david-crespo david-crespo force-pushed the tailwind-reset-order branch from 40d15c8 to 7e1b28d Compare March 6, 2024 03:04
],
patterns: [
// import all CSS except index.css at top level through CSS @import statements
// to avoid bad ordering situations. See https://github.com/oxidecomputer/console/pull/2035
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

converted this file to JS solely so I could put this comment here

.DialogOverlay {
/* background: hsla(0, 0%, 0%, 0.33); */
@apply fixed inset-0 z-10 overflow-auto bg-scrim;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Turned this into a DialogOverlay component.

@david-crespo david-crespo changed the title Fix CSS import ordering Centralize CSS imports for explicit ordering Mar 6, 2024
@david-crespo david-crespo marked this pull request as ready for review March 6, 2024 03:36
@david-crespo david-crespo changed the title Centralize CSS imports for explicit ordering Centralize CSS all imports in index.css for explicit ordering Mar 6, 2024
onClick={onDismiss}
aria-hidden
/>
<DialogOverlay />
Copy link
Collaborator Author

@david-crespo david-crespo Mar 6, 2024

Choose a reason for hiding this comment

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

I can't find any problem with removing this onClick and the pointer-events-auto. Everything works – clicking out to close the modal, scrolling popovers when they're inside the modal, etc. Give it a try @benjaminleonard.

@david-crespo david-crespo changed the title Centralize CSS all imports in index.css for explicit ordering Centralize all CSS imports in index.css for explicit ordering Mar 6, 2024
Copy link
Contributor

@charliepark charliepark left a comment

Choose a reason for hiding this comment

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

Clicking through, everything looks good to me

@david-crespo david-crespo merged commit 784e8aa into main Mar 6, 2024
@david-crespo david-crespo deleted the tailwind-reset-order branch March 6, 2024 18:22
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.

3 participants