Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

CSS define:vars should only add definitions to top-level components #855

Closed
1 task done
mikestopcontinues opened this issue Jan 26, 2024 · 4 comments
Closed
1 task done

Comments

@mikestopcontinues
Copy link

Astro Info

Astro                    v4.2.4
Node                     v20.11.0
System                   macOS (arm64)
Package Manager          pnpm
Output                   hybrid
Adapter                  @astrojs/vercel/serverless
Integrations             auto-import
                         @astrojs/mdx
                         astro-icon
                         @astrojs/partytown
                         @astrojs/sitemap
                         astro-robots-txt

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

Right now, <style define:vars={{}}> adds the variable definitions to every single element in the component. This drastically increases the footprint of components, it's difficult to read, and it will cause issues if a user intentionally wants to override a local variable within the component itself.

What's the expected result?

The CSS var definitions should only be added to the top-level components.

This issue was closed before it was discussed in withastro/astro#7328.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-sdeict-dc8tm6?file=src%2Fcomponents%2FDefineVars.astro

Participation

  • I am willing to submit a pull request for this issue.
@lilnasy
Copy link
Contributor

lilnasy commented Jan 26, 2024

define:vars is weird. It only works on the html element the compiler notices on the file. In a page with only components and no html tags, it has no effect at all.

I want to see it deprecated altogether. Maybe we can offer a function as a replacement.

-<div>
+<div style={defineVars({ color })}>
  1111
  <button type="button">button</button>
</div>

-<style define:vars={{ color }}>
+<style>
  button {
    color: var(--color)
  }
</style>

@mikestopcontinues
Copy link
Author

I'm all for it!

@imaginarny
Copy link

I would also like to draw attention to the behavior when you actually set define:vars along with the is:inline directive on a style tag.

<style is:inline define:vars={{ cssVar: 'example' }}>...</style>

Compiles to:

<style>--cssVar: 'example';...</style>

This way, you can't use the variable as it was inlined as-is right into the style tag. All you need would be if it either:

  • added :root {} wrapper around the variables
  • or offered some way/option that lets you define your own element/wrapper

The first option would be perfect for use in <head> with inline styles, for example, when you want to set something immediately that you also preload. You can achieve this another (better) way, though.

Although, I don't see a use case for it outside of the <head>. Just wanted to point out another "questionable" behavior while I was trying to pass a variable used in head and found this open issue among the many closed ones.


I want to see it deprecated altogether. Maybe we can offer a function as a replacement.

In the case of deprecating it, how would the function work for the <script> tag?

@lilnasy
Copy link
Contributor

lilnasy commented Feb 5, 2024

In the case of deprecating it, how would the function work for the <script> tag?

<script define:vars> already has a replacement lined up

@lilnasy lilnasy transferred this issue from withastro/astro Feb 26, 2024
@withastro withastro locked and limited conversation to collaborators Feb 26, 2024
@lilnasy lilnasy converted this issue into discussion #856 Feb 26, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants