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.

Sign up for GitHub

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

Jump to bottom

Resolve Opacity Bug #881

Merged
merged 2 commits into from
Feb 21, 2020
Merged

Resolve Opacity Bug #881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e59aa6
Resolve bug where opacities defined in percentages are being reduced …
genemecija Feb 21, 2020
8a0a310
Added test to verify preservation of opacities defined as percentages
genemecija Feb 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/postcss-convert-values/src/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ test(
processCSS('h1{width:100.00%}', 'h1{width:100%}')
);

test(
'should preserve opacities defined as percentages',
passthroughCSS('h1{opacity:100%}')
);

Copy link
Member

Choose a reason for hiding this comment

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

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
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

test('should not mangle flex basis', passthroughCSS('h1{flex-basis:0%}'));

test('should not mangle flex basis (2)', passthroughCSS('h1{FLEX-BASIC:0%}'));
Expand Down
2 changes: 1 addition & 1 deletion packages/postcss-convert-values/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function clampOpacity(node) {
}
let num = Number(pair.number);
if (num > 1) {
node.value = 1 + pair.unit;
node.value = pair.unit === '%' ? num + pair.unit : 1 + pair.unit;
} else if (num < 0) {
node.value = 0 + pair.unit;
}
Expand Down