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

Fix at-rules nesting #216

Merged
merged 2 commits into from
May 30, 2017
Merged

Fix at-rules nesting #216

merged 2 commits into from
May 30, 2017

Conversation

giuseppeg
Copy link
Collaborator

Fixes #214

@thysultan
Copy link
Contributor

LGTM, also patched the @media prefixing in v3.0.6

span {}
      @media (min-width: 480px) { div { color: red } }

/* @media[prefix](min-width[prefix]:480px[prefix]) div[prefix]{color:red} */

@giuseppeg
Copy link
Collaborator Author

@thysultan thank you! I found another small bug

@-webkit-keyframes -[prefix]{0%{opacity:0}100%{opacity:1}}@keyframes -[prefix]{0%{opacity:0}100%{opacity:1}}

animations get an extra - :(

@thysultan
Copy link
Contributor

I think that's by design since the key for name-spacing animations is extracted from the namespace and prefixed with a - incase the extracted key is a number, what is the input vs expected?

@thysultan
Copy link
Contributor

In contrast the same key will be applied to the animation/animation-name property

@thysultan
Copy link
Contributor

thysultan commented May 30, 2017

@giuseppeg Actually the way it handles it right now looks like incorrect behavior since it prevents you from using an empty namespace to mimic a global namespace with the guarantee that keyframe names are preserved. patched in v3.0.7

@giuseppeg
Copy link
Collaborator Author

giuseppeg commented May 30, 2017

@thysultan cool, however now (with 3.0.7) namespaced one don't get the prefix:

@keyframes hahaha {}
/* result @keyframes hahaha- {} */
/* expected @keyframes hahaha-[data-jsx="123"] {} */

I used [data-jsx="123"] or [data-jsx~="123"] as prefix which is invalid and can't be used in animations names unfortunately. Maybe you can hash those for animations?

@giuseppeg
Copy link
Collaborator Author

technically you could also do prefix.replace(/\W/g, '') instead of hashing

@thysultan
Copy link
Contributor

thysultan commented May 30, 2017

For attributes it grabs just the part after = within quotes, then it converts every invalid character to a valid character. In this case numbers as leading characters are invalid so they are all converted to a single dash but there is also \s.#~+>@ as invalid characters, ex. putting a non-numeric character in front of those numbers will make them valid.

I wonder if we can avoid removing the leading number rule since the key is appended to the authored animation name making it valid in that context.

@giuseppeg
Copy link
Collaborator Author

giuseppeg commented May 30, 2017

I wonder if we can avoid removing the leading number rule since the key is appended to the authored animation name making it valid in that context.

Yes that's what I thought too, a string that matches the character class \w prefixed with the original (supposedly valid) animation name should be fine. It is not our problem if a user gives an invalid name to the animation.

I am fine with any solution :) as soon as it works with our data attributes too (whose value is numeric) – in general I think that this is a valid fix for the library in general.

@thysultan
Copy link
Contributor

patched, v3.0.9

@giuseppeg giuseppeg merged commit f9b6dcb into master May 30, 2017
@giuseppeg giuseppeg deleted the fix-at-rules branch May 30, 2017 18:20
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.

2 participants