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(postcss-convert-values): preserve percentage-only properties #1212

Merged
merged 2 commits into from Oct 21, 2021

Conversation

ludofischer
Copy link
Collaborator

Don't process properties that look like numbers, but only take percentage values
according to the spec.

Fix #1203

I've found that the original code seems to repeat similar checks in in different places, so I've found the logic a bit confusing, but I have avoided changing too much.

@@ -18,6 +18,23 @@ const LENGTH_UNITS = [
'pc',
'px',
];

// These properties only accept percentages, so no point in trying to transform
const notANumber = new Set([
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are there more of these? Could not find a list of CSS properties grouped by accepted values

@@ -71,7 +88,7 @@ function clampOpacity(node) {
}
}

function shouldKeepUnit(decl) {
function shouldKeepZeroUnit(decl) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have changed this function name because it is only used in the parseWord() function when num === 0.

if (
~lowerCasedProp.indexOf('flex') ||
lowerCasedProp.indexOf('--') === 0 ||
notANumber.has(lowerCasedProp)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Skip processing completely here, since there is not meaningful possible conversion.

Don't process properties that look like numbers, but only take percentage values
according to the spec.

Fix #1203
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #1212 (f48c0dd) into master (f199323) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1212   +/-   ##
=======================================
  Coverage   96.53%   96.54%           
=======================================
  Files         116      116           
  Lines        3611     3614    +3     
  Branches     1063     1064    +1     
=======================================
+ Hits         3486     3489    +3     
  Misses        117      117           
  Partials        8        8           
Impacted Files Coverage Δ
packages/postcss-convert-values/src/index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f199323...f48c0dd. Read the comment docs.

@ludofischer ludofischer merged commit 8f34538 into master Oct 21, 2021
@ludofischer ludofischer deleted the fix-percentages branch October 21, 2021 11:35
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.

[Bug]: descent-override and ascent-override are expecting <percentage> for zero values
3 participants