Resolve Opacity Bug

[genemecija]

Resolving bug in which opacity values defined as percentages are being converted to 1%.

[codecov-io]

Codecov Report

Merging #881 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #881   +/-   ##
=======================================
  Coverage   97.27%   97.27%           
=======================================
  Files         118      118           
  Lines        3452     3452           
  Branches     1035     1036    +1     
=======================================
  Hits         3358     3358           
  Misses         86       86           
  Partials        8        8

Impacted Files	Coverage Δ
packages/postcss-convert-values/src/index.js	100% <100%> (ø)	⬆️

---

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 613d562...8a0a310. Read the comment docs.

[anikethsaha]

Good Catch 👍

Can you add few tests in postcss-convert-values for future use.

[anikethsaha]

Can you add some more.

may be like these
⬇️

test(
  'should convert the opacity to 1px',
  processCSS('h1{opacity : 80px}', 'h1{opacity: 1px}')
);
test(
  'should convert the opacity from 80% -> 80%',
  processCSS('h1{opacity :80%}', 'h1{opacity :80%}')
);

Also check this one
⬇️

test(
  'should convert the opacity without any unit',
  passthroughCSS('h1{opacity :80}')    // expected is 80
);

[genemecija]

Regarding your tests:

test(
  'should convert the opacity to 1px',
  processCSS('h1{opacity : 80px}', 'h1{opacity: 1px}')
);

'px' is not a valid unit, and I opted not to check for invalid units as that's not the tool's purpose (minification vs linting)

---

test(
  'should convert the opacity from 80% -> 80%',
  processCSS('h1{opacity :80%}', 'h1{opacity :80%}')
);

If I'm not mistaken, processCSS('h1{opacity :80%}', 'h1{opacity :80%}') would be the same test as passthroughCSS('h1{opacity:80%}') or the test I created, passthroughCSS('h1{opacity:100%}'). Please correct me if I'm wrong about that.

---

test(
  'should convert the opacity without any unit',
  passthroughCSS('h1{opacity :80}')    // expected is 80
);

Any number above '1.0' is invalid and would be converted to '1'. There's a check for this at line 281:

test(
  'should clamp opacity to 1 maximum',
  processCSS(
    'h1{opacity:150;opacity:15;opacity:1.5}',
    'h1{opacity:1;opacity:1;opacity:1}'
  )
);

---

The only other tests I can think that aren't already tested for are for validating unit type, but that would be a linter's job.

[anikethsaha]

Regarding your tests:

test(
 'should convert the opacity to 1px',
 processCSS('h1{opacity : 80px}', 'h1{opacity: 1px}')
);

'px' is not a valid unit, and I opted not to check for invalid units as that's not the tool's purpose (minification vs linting)

Invalid CSS should lean to either no minification or invalid minification. This is an example of invalid minification.
You can keep it or omit it. either looks good.
but I would check the behavior by keeping it for future use

If I'm not mistaken, processCSS('h1{opacity :80%}', 'h1{opacity :80%}') would be the same test as passthroughCSS('h1{opacity:80%}') or the test I created, passthroughCSS('h1{opacity:100%}'). Please correct me if I'm wrong about that.

Yea, but I prefer using processCSS as its more visible. again your choice to use either of them. but they both are fine 👍

Any number above '1.0' is invalid and would be converted to '1'. There's a check for this at line 281:

if its done, leave it then 👍

The only other tests I can think that aren't already tested for are for validating unit type, but that would be a linter's job.

Feel free to choose whether to test it or not. @evilebottnawi what you think?

[alexander-akait]

I prefer to use passthroughCSS when values was not changed, I think in future we refactor tests on snapshots, because it is more easy to read

[alexander-akait]

What is an original problem, I lose context, can you provide example? Or a link on issue?

[anikethsaha]

What is an original problem, I lose context, can you provide example? Or a link on issue?

This is the current behavior

opacity : 40% -> opacity : 1%

It is wrong minification

[alexander-akait]

@genemecija opacity : 40% is invalid css syntax, why you need that line?

[anikethsaha]

@genemecija opacity : 40% is invalid css syntax, why you need that line?

@evilebottnawi I think this is an correct syntax. I tried on developer tools. its working

[alexander-akait]

hm, yes, you are right https://developer.mozilla.org/en-US/docs/Web/CSS/opacity, never use % 😄

[alexander-akait]

Thanks!

[herecydev]

@evilebottnawi was there a release for this patch?

[alexander-akait]

@herecydev yes, in future, still a lot of work

[SylvainGarrigues]

Any possibility for a hot bug fix release instead of waiting for a new formal release with other features? This is breaking UI for sites in production which rely on cssnano.

X% (unminified) -> 1% (minified) means elements become invisible in production builds and not in development builds. Can drive people crazy.

[anikethsaha]

it will be done soon.
in the meantime use postcss-ignore-plugin or I would suggest disabling this plugin

[svendlarsen]

@anikethsaha @evilebottnawi Please reconsider making a separate patch release for this bugfix! Three months is a long time between merge and release for a fix of this nature.

[anikethsaha]

it will be released soon, #881 (comment)

[klesun]

um, bump? Guys, your lib is used by a decent amount of people, would you consider making the hotfix release asap, please? Is 4.0.0-nightly.2020.8.10 behind 4.1.10?

[anikethsaha]

Please use cssnano@nightly in the meantime

[anikethsaha]

nightly is up-to-date with the master