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

chore: add tailwind styles to lit migration #4804

Closed
wants to merge 12 commits into from

Conversation

fpbrault
Copy link
Contributor

@fpbrault fpbrault commented Dec 19, 2024

Significant File Size Improvements!

Before

We can't directly compare with atomic-text, as it previously had no Tailwind classes. Instead, we’ll examine atomic-badge, a relatively similarly sized component.

File: atomic-result-badge.js

  • Normal: 45 kB
  • Gzipped: 7.63 kB

All of the Tailwind styles for all components in the repo are included in every component that uses at least one Tailwind class. For atomic-result-badge.js, these styles account for 41,137 bytes. Removing them gives a clearer picture of the base file size:

  • Normal (without styles): 4.1 kB
  • Gzipped (without styles): 1.25 kB

After

File: atomic-text.js

  • Only inline styles are included in this file.
  • Normal: 6 kB
  • Gzipped: 1.8 kB

File: tailwind.global.tw.css.js

  • Loaded once and shared across all components.
  • Includes all Tailwind styles used in Atomic, not just those for the atomic-text component.
  • Normal: 22 kB
  • Gzipped: 5.26 kB

File: coveo.tw.css.js

  • Loaded once and shared across all components.
  • Contains the default Atomic theme (CSS variables only).
  • Normal: 1.1 kB
  • Gzipped: 462 B

File: atomic-text.styles.tw.css.js

  • Styles specific to the atomic-text component.
  • Normal: 174 B
  • Gzipped: 140 B

Copy link

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 242.9 242.9 0
commerce 353.9 353.9 0
search 414 414 0
insight 405.2 405.2 0
recommendation 255.1 255.1 0
ssr 407.4 407.4 0
ssr-commerce 371 371 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

Comment on lines +1 to +9
div {
transition: background-color 0.3s;

@apply text-white;
}

div:hover {
@apply bg-primary-dark;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These styles are just to demonstrate how to combine normal css, inline styles, tailwind @apply rules and tailwind classes directly in the component markup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For Tailwind V4, the theme need to be imported with the theme(reference) rule so that the tailwind utilities (including our custom theme) are available. See this for example:
https://github.com/coveo/headless-lit/blob/tsc-tw4/lito/src/components/lito-result-title/lito-result-title.css
(Notice that the theme.css file in that example only contains @theme rules, as other rules cannot be referenced that way)

It is possible to import the theme directly or even tailwind itself instead, but that will result in all their css being embedded in this file.

Comment on lines +39 to +49
static styles: CSSResultGroup = [
TailwindLitElement.styles,
css`
div {
border: 1px solid red;
border-radius: var(--atomic-border-radius-xl);
}
`,
unsafeCSS(styles),
];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't use @apply rules in inline stylesheets

@apply rounded-xl;

Can still use css variables, but I don't see a good reason to do this instead of an external stylesheet or tailwind classes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a component does not have a separate stylesheet, no need to add this, as long as the component extends TailwindLitElement tailwind classes will work!

Comment on lines 22 to 28
plugins: [
new webpack.NormalModuleReplacementPlugin(/\.tw\.css$/, function (
resource
) {
resource.request = resource.request + '.js';
}),
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed so webpack can handle the .css.js imports

@fpbrault fpbrault changed the base branch from lab/migrate-one to master January 15, 2025 15:39
@fpbrault fpbrault force-pushed the lab/migrate-one-styles branch from c90928b to fb0dab6 Compare January 15, 2025 15:50
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.

1 participant