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: avoid parsing animation: "none" to animation: "none" none. #295

Merged
merged 2 commits into from
Sep 17, 2022

Conversation

yisibl
Copy link
Contributor

@yisibl yisibl commented Sep 17, 2022

In the animation property, special handling is required when the values of both animation-name and animation-fill-mode are none.

Input

.foo {
    animation: "none";
}

.bar {
  animation: "none" 2s;
}

Before

.foo {
  animation: "none" none;
}

.bar {
  animation: "none" 2s none;
}

After

.foo {
  animation-name: "none";
}

.bar {
  animation: "none" 2s;
}

In the `animation` property, special handling is required when the values of both `animation-name` and `animation-fill-mode` are `none`.
@devongovett devongovett merged commit 0732197 into parcel-bundler:master Sep 17, 2022
@devongovett
Copy link
Member

Thanks. I updated the logic slightly in df280dc and b6eec64, so that we serialize the name as a quoted string within the animation shorthand whenever it conflicts with a keyword from one of the other properties.

@yisibl yisibl deleted the fix-animation-name-none branch September 18, 2022 03:23
@yisibl
Copy link
Contributor Author

yisibl commented Sep 18, 2022

@devongovett Nice work, but what is the reason for serializing the animation-name to the end?

Also, we might want to consider browsers that don't support animation-name: <string>, like Chrome is implementing in: https://chromium-review.googlesource.com/c/chromium/src/+/3865188

@devongovett
Copy link
Member

Moving to the end fixed tests like this:

animation: 2s alternate reverse

In this case, alternate is the direction, and reverse is the name. Without quotes, it's ambiguous so order is important.

Hadn't considered that strings were a new syntax though. Might need to update this...

@devongovett
Copy link
Member

Updated: 6e47cdb

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