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

test: clearer test case for 966 #974

Closed
wants to merge 1 commit into from

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Jul 18, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Follow up to #973

Breaking Changes

Additional Info

@jquense
Copy link
Contributor Author

jquense commented Jul 18, 2019

Let me try and figure out what is different about this vs my setup @evilebottnawi

@alexander-akait
Copy link
Member

alexander-akait commented Jul 18, 2019

I see the problem, when you use localIdentName we escape all special CSS characters, but when you use getLocalIdent we output returned value without escaping, maybe you are right and it is bug, i think we should use the same logic for both cases

@alexander-akait
Copy link
Member

We escape all characters in placeholder excluding [locals]

@jquense
Copy link
Contributor Author

jquense commented Jul 18, 2019

AHHHH you're right! That's the problem it assumes that the local name is already css safe. I think it's safe to make that change? Or would it not be ok?

@alexander-akait
Copy link
Member

@jquense i think we should always escape non standard CSS character in both cases, why? because you path can contains invalid characters for CSS, localIdentName already works as expected, but getLocalIdent - no

@jquense jquense closed this Oct 1, 2019
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.

None yet

2 participants