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

Define css variables, do not add to the style attribute of each tag #7328

Closed
1 task
J-env opened this issue Jun 8, 2023 · 7 comments
Closed
1 task

Define css variables, do not add to the style attribute of each tag #7328

J-env opened this issue Jun 8, 2023 · 7 comments

Comments

@J-env
Copy link

J-env commented Jun 8, 2023

What version of astro are you using?

2.6.1

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm,pnpm

What operating system are you using?

Mac

What browser are you using?

Chrome

Describe the Bug

// css-test.astro

---
const color = 'red';
---

<div>
  1111
  <button type="button">button</button>
</div>

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

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.
@J-env
Copy link
Author

J-env commented Jun 8, 2023

Duplicate style attribute. Each tag has a common outer tag

project:

image

@matthewp
Copy link
Contributor

matthewp commented Jun 9, 2023

I assume we are doing it this way because this avoids --color from bleeding into child components. I'm not really sure though. Anyways, if you wanted to submit a PR that changed how this work we could discuss there. We like to keep the issue tracker to bugs, and this is an intentional design decision, so going to close. Happy to continue discussing here though!

@matthewp matthewp closed this as completed Jun 9, 2023
@J-env
Copy link
Author

J-env commented Jun 9, 2023

---
const color = 'red';
---

<div>
  1111
  <button type="button">button</button>
</div>

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

build:

<style>
button.astro-hash {
  color: var(--color-hash);
}
<style>

@matthewp
Copy link
Contributor

matthewp commented Jun 9, 2023

@J-env Where is it defined to red in this example?

@J-env
Copy link
Author

J-env commented Jun 9, 2023

@matthewp The outermost element of the component

buid:

<div style="--color-hash: red;">
  1111
  <button type="button">button</button>
</div>

<style>
button.astro-hash {
  color: var(--color-hash);
}
<style>

@clockworkroger
Copy link

clockworkroger commented Sep 13, 2023

It's an ongoing issue, one that definitely needs reconsideration.

I think only setting --color on the outermost HTML element is a perfectly acceptable and expected use of CSS custom properties. I don't even think it needs to be hashed, because it's already scoped to that specific element, going into the inline style tag like that.

Arguably, you're doing more work by outputting that style attribute on <div> and <button> inside it the way Astro is now.

Let me throw another use-case out there...

I have a component for a section to which I'm adding a background image (three, actually, for responsive lazy loading). If I use define:vars to create a custom property so that my background image URL from getImage() can be used for that component, I get a style= attribute on my outermost <section> tag.... and every <div>, <h3>, <p>, and <a> tag inside it. None of those tags need the custom property, because CSS already handles that if for some reason they do, but it's now defined for every single one of them, and my HTML is that much more bloated. Because of that, I'm adding my own style= attribute on my outermost tag.... it's just not as clean as if I were following the example laid out in the docs.

More generically, if I make a Card component with a customColor property and in its CSS, I have a button background color, text color, and border color all using the same var(--custom-color), I'd expect that my outermost Card <li> or <div> (whatever it is) has a style="--custom-color: red" attribute on it, and that's it. A little like how you might have to explicitly pass properties or content down through multiple components if you want to set something on the deepest one.

And I understand that it's almost antithetical to the whole idea of Astro, but it feels like it simplifies things in an intuitive way.

(Full disclosure, I'm still somewhat new to Astro, so maybe I'm just doing things entirely wrong. Only really started using it a couple weeks ago for a project here at work. But I gotta say, I really like it so far.)

@mikestopcontinues
Copy link

This is definitely the wrong behavior. In addition to ballooning components, it would actually get in the way of trying to intentionally override variables within the component itself.

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

No branches or pull requests

4 participants