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

Allow hypenated variables in interpolation #120

Closed
wants to merge 3 commits into from

Conversation

FabianMeul
Copy link

Hi,

I've noticed that media-feature-value-dollar-variable does not account for hyphenated variables in the regex. This seems to originate from a slight oversight when composing the interpolationVarRegex.

@mixin respond-lg {
  // This throws an error
  // Expected a dollar-variable (e.g. $var) to be used as a media feature value
  @media (min-width: #{$bp-lg}) {
    @content;
  }
}

@mixin respond-to ($bp) {
  // This works just fine
  @media (min-width: #{$bp}) {
    @content;
  }
}

@dryoma
Copy link
Collaborator

dryoma commented May 10, 2017

Good call! Does it still work with $var-1px, which is not a variable, but an expression?

@FabianMeul
Copy link
Author

FabianMeul commented May 10, 2017

It'll match that expression too, yes. All I did is match the regex to the one that is used for the variable detection.

If $var-1px should throw a warning though, I don't know how we can spot the difference (syntax-wise) between $my-var and $var-1pxusing a "simple" regex.

Also: Should the regex take into account the regex that is specified in dollar-variable-pattern?

@dryoma
Copy link
Collaborator

dryoma commented May 10, 2017

Well, yeah, that is an oversight, as it seems. On the other hand, we have a somewhat robust but a bit messy Sass value parser that can tell such things apart. I think it could be used here as well, at least for checking if the - is an operator or just a dash in a variable name. So if you'd like to dig a bit deeper, we're perfectly ok if this PR gets some more commits :)

And one more thing: tests are always a great thing :) So it would be nice if you added some tests here as well.

@FabianMeul
Copy link
Author

Well, I added the extra check and some tests, but seem to have broken some others...

I don't really see where it goes wrong though, can you perhaps provide me with some instructions? :-)

@XhmikosR
Copy link
Member

Closing since this outdated. Feel free to make a new PR.

@XhmikosR XhmikosR closed this Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants