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

Expand variables in AtRule properties #104

Conversation

pvande
Copy link
Contributor

@pvande pvande commented Aug 31, 2019

As written, this plugin makes the seemingly reasonable assumption that all variable expansions will happen inside of a CSS Rule (that is, a set of Properties scoped to one or more Selectors).

However, some CSS At-Rules (like @font-face and @page) allow for CSS Properties to be their direct descendants. These are philosophically a little trickier to implement, since (e.g.) the @page [At-]Rule doesn't actually appear in the DOM hierarchy, but not permitting At-Rules to use variables limits their utility and forces duplication of variable values.

:root {
  --theme-color: red;
}

body {
  color: var(--theme-color);
}

@page {
  background-color: var(--theme-color);  /* Never expanded. */
}

For such cases, there is a simple, sensible interpretation that extends naturally: :root implies the set of "global" variables, and the expectation is that At-Rules would have access to variables defined in that scope. Variables defined in lower scopes should (obviously) not have an effect on such high-level [At-]Rules – and the existing behaviors for At-Rule-scoped Rules seem adequate for describing the cases where both an At-Rule and a lower-scoped variable need to coordinate.

// We only need to split if is actually comma separted(selectors > 1)
if(rule.selectors.length > 1) {
// We only need to split if it's actually a Rule with multiple selectors
if(rule.type == 'rule' && rule.selectors.length > 1) {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's use rule.type === 'rule'

Also why are we moving away from walkRules if we then check for rule.type === 'rule' here anyway?

Copy link
Owner

Choose a reason for hiding this comment

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

It looks like it is because walkRules doesn't walk over atrule types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I believed I had responded to this comment. Yes, it's because atrule types were not being traversed by walkRules.

MadLittleMods added a commit that referenced this pull request Apr 24, 2020
@MadLittleMods
Copy link
Owner

Hey @pvande, I've created #121 with the review comments resolved 🚀

Thanks for the contribution and sorry for the delay 🙇

MadLittleMods added a commit that referenced this pull request Apr 24, 2020
MadLittleMods added a commit that referenced this pull request Apr 24, 2020
Repository owner deleted a comment Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants