Feat: postcss lowercase text #867

anikethsaha · 2020-01-16T05:54:37Z

Moved postcss-lowercase-text to cssnano-preset-default
Updated few integration tests.

codecov-io · 2020-01-16T05:58:53Z

Codecov Report

Merging #867 into master will increase coverage by 0.00%.
The diff coverage is 97.53%.

@@           Coverage Diff           @@
##           master     #867   +/-   ##
=======================================
  Coverage   97.28%   97.28%           
=======================================
  Files         118      123    +5     
  Lines        3458     3539   +81     
  Branches     1040     1054   +14     
=======================================
+ Hits         3364     3443   +79     
- Misses         86       88    +2     
  Partials        8        8

Impacted Files	Coverage Δ
packages/cssnano-preset-default/src/index.js	`100.00% <ø> (ø)`
packages/postcss-lowercase-text/src/atRules.js	`88.23% <88.23%> (ø)`
...wercase-text/src/__tests__/data/pseudo-elements.js	`100.00% <100.00%> (ø)`
packages/postcss-lowercase-text/src/index.js	`100.00% <100.00%> (ø)`
packages/postcss-lowercase-text/src/selector.js	`100.00% <100.00%> (ø)`
packages/postcss-lowercase-text/src/units.js	`100.00% <100.00%> (ø)`
... and 1 more

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 8dd5c44...da03b82. Read the comment docs.

anikethsaha · 2020-01-28T08:00:19Z

cc @evilebottnawi

packages/postcss-lowercase-text/src/__tests__/data/pseudo-elements.js

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

alexander-akait

Looks good, we need more improvement - let's do it

packages/postcss-lowercase-text/package.json

packages/cssnano-preset-default/package.json

packages/postcss-lowercase-text/README.md

alexander-akait

Can we add more tests?

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

anikethsaha · 2020-02-04T10:37:41Z

packages/postcss-lowercase-text/src/__tests__/index.test.js

+  run(
+    'should not transform css variable declarations',
+    ':root {  --BG-COLOR: coral; --TEXT-COLOR: RED;}',
+    ':root {  --BG-COLOR: coral; --TEXT-COLOR: red;}'


they are css values, they can be transformed as well just like the rest of the values.

Like this

https://github.com/cssnano/cssnano/pull/867/files#diff-fe88a57283a0a1a5c792a3000e5badbaR43-R53

I will read spec, maybe you are right

anikethsaha · 2020-02-04T10:58:58Z

@evilebottnawi Can you please check this allowed transformation

 [at rule]            [allowed lowercasing]
  keyframes -> nameAndPropsTransform,
  counter-style  -> namePropsTransform,
  namespace  ->  nameTransformOnly,
  import  ->  nameTransformOnly,
  font-face  ->   nameAndPropsTransform,
  font-feature-values   ->  nameTransformOnly,
  page   ->  nameAndPropsTransform,
  supports   ->   nameAndPropsTransform,
  media  ->   nameParamsPropsTransform,
  charset  ->  nameTransformOnly,
  document  ->   nameTransformOnly,
  viewport  ->   nameAndPropsTransform,

alexander-akait · 2020-03-24T10:19:39Z

Need fix CI

anikethsaha · 2020-03-24T10:21:35Z

Need fix CI

working on it

anikethsaha · 2020-04-27T08:13:04Z

cc @evilebottnawi finally the CI is green. Can you review this.
we need to rebase other PR in order to fix their CI as there is some issue with @babel/preset-env and node13 for which I had to update it to 7.9 so after merging this, we need to rebase other PR.

alexander-akait · 2020-04-28T10:27:15Z

jest.config.js

@@ -9,6 +9,7 @@ module.exports = {
    '_processCSS.js',
    '_processCss.js',
    '_webpack.config.js',
+    'packages/postcss-lowercase-text/src/__tests__/data/',


Why it here?

jest was running the test running in this directory as well as there is an .js file which is true for jest's default test file pattern. So I added them. this directory should not be tested as it has data only.

alexander-akait · 2020-04-28T10:28:03Z

packages/cssnano-preset-default/package.json

@@ -24,6 +24,7 @@
    "postcss-discard-duplicates": "^4.0.2",
    "postcss-discard-empty": "^4.0.1",
    "postcss-discard-overridden": "^4.0.1",
+    "postcss-lowercase-text": "*",


Use 1.0.0 verison

I didnt publish v1 . so it will through error if we build it locally. I will release it manually after merging of this.

alexander-akait · 2020-04-28T10:28:19Z

packages/cssnano-util-get-match/package.json

@@ -24,6 +24,6 @@
    "url": "https://github.com/cssnano/cssnano/issues"
  },
  "engines": {
-    "node": ">=10.13"
+    "node": ">=10.13.0"


No need .0

alexander-akait · 2020-04-28T10:29:08Z

packages/cssnano/src/__tests__/postcss-calc.js

@@ -1,5 +1,7 @@
 import processCss from './_processCss';

+jest.setTimeout(30000);


Avoid it and use setupFilesAfterEnv https://jestjs.io/docs/en/jest-object#jestsettimeouttimeout

ok cool, I will try this 👍

alexander-akait · 2020-04-28T10:30:04Z

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

+    '@MEDIA SCREEN and (min-width: 480px){}',
+    '@media screen and (min-width: 480px){}'
+  );
+});


Please add test: @MEDIA SCREEN AND (MIN-WIDTH: 480PX) {}

ok cool 👍

alexander-akait · 2020-04-28T10:31:44Z

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

+  run(
+    'should safely transform the @charset',
+    '@CHARSET "iso-8859-15";',
+    '@charset "iso-8859-15";'


Do not touch charset never, it should b always in lowercase, we should keep @CHARSET as is

is it case sensitive?

alexander-akait · 2020-04-28T10:32:10Z

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

+    'should  transform the @import ',
+    `@IMPORT url("fineprint.css") print;@import url("bluish.css") speech;@ImPoRt 'custom.css';@import url("CHROME://communicator/SKIN/");@import "common.css" screen;`,
+    `@import url("fineprint.css") print;@import url("bluish.css") speech;@import 'custom.css';@import url("CHROME://communicator/SKIN/");@import "common.css" screen;`
+  );


Please add test: @IMPORT URL("fineprint.css") PRINT;

ok cool 👍

alexander-akait · 2020-04-28T10:33:19Z

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

+  run(
+    'should safely transform the @namespace',
+    '@NAMESPACE prefix url(http://www.w3.org/1999/xhtml);',
+    '@namespace prefix url(http://www.w3.org/1999/xhtml);'


Please add test @NAMESPACE SVG URL(http://www.w3.org/2000/svg);, SVG should be in uppercase (keep as is)

alexander-akait · 2020-04-28T10:33:42Z

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

+  run(
+    'should safely transform the @supports ',
+    '@SUPPORTS (display: grid) {div {display: grid;}};@supports not (display: GRID) {div {float: right;}}',
+    '@supports (display: grid) {div {display: grid;}};@supports not (display: GRID) {div {float: right;}}'


Please add test: @SUPPORTS (DISPLAY: GRID) {}

alexander-akait · 2020-04-28T10:34:44Z

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

+  run(
+    'should safely transform the @document  ',
+    '@DOCUMENT URL("https://www.EXAMPLE.com/") {h1 {COLOR: green;}}',
+    '@document URL("https://www.EXAMPLE.com/") {h1 {color: green;}}'


URL should be lowercased

@evilebottnawi, it will lowercase the argument ("https://www.EXAMPLE.com/") as well which is not saved as lowercasing anything after domain name in URLs are not safe.
postcss will parse URL("https://www.EXAMPLE.com/") whole as params so we cant lowercase only URL.

let me know what you think!

alexander-akait · 2020-04-28T10:35:16Z

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

+  run(
+    'should safely transform the @page   ',
+    '@PAGE {margin: 1cm;};@page :FIRST {margin: 2cm;}',
+    '@page {margin: 1cm;};@page :FIRST {margin: 2cm;}'


Please add test: @PAGE :FIRST (all should be lowercased)

alexander-akait · 2020-04-28T10:36:17Z

packages/postcss-lowercase-text/src/__tests__/atRules.test.js

+  run(
+    'should safely transform the @keyframes',
+    '@KEYFRAMES slidein {from {transform: translateX(0%);}};@KEYFRAMES important2  {FROM {TRANSFORM: translateX(0%);}}',
+    '@keyframes slidein {from {transform: translateX(0%);}};@keyframes important2  {from {transform: translateX(0%);}}'


Please add test - @KEYFRAMES SLIDEIN {}, SLIDEIN should not be lowercased (keep as is)

alexander-akait · 2020-04-28T10:38:30Z

packages/postcss-lowercase-text/src/__tests__/data/pseudo-elements.js

+  },
+};
+
+export default pseudoElementsList;


Can we write a script and get it from mdn?

I don't know of any API to request from MDN !

can you share any link.

alexander-akait · 2020-04-28T10:41:16Z

packages/postcss-lowercase-text/src/__tests__/index.test.js

+  run(
+    'should safely transform the  CSS attribute Selectors to lowercase',
+    '[NAME="newname"]{border: 1px solid black;}',
+    '[name="newname"]{border: 1px solid black;}'


Please add tests:

A[HREF*="insensitive" I] -> a[href*="insensitive" i]

A[HREF*="insensitive" S] -> a[href*="insensitive" s]

alexander-akait · 2020-04-28T10:41:44Z

packages/postcss-lowercase-text/src/__tests__/index.test.js

+  run(
+    'should safely transform the  CSS attribute Selectors to lowercase',
+    '[NAME="newname"]{border: 1px solid black;}',
+    '[name="newname"]{border: 1px solid black;}'


Please add test: DIV:NOT([LANG])

alexander-akait · 2020-04-28T10:43:36Z

packages/postcss-lowercase-text/src/__tests__/index.test.js

+    'should safely transform the  CSS attribute Selectors to lowercase',
+    '[NAME="newname"]{border: 1px solid black;}',
+    '[name="newname"]{border: 1px solid black;}'
+  );


Please add test: LI[DATA-VALUE|="1900"], we should not lowercase DATA-VALUE (keep as is) because https://html.spec.whatwg.org/multipage/dom.html#embedding-custom-non-visible-data-with-the-data-*-attributes

alexander-akait · 2020-04-28T10:44:29Z

packages/postcss-lowercase-text/src/__tests__/index.test.js

+  run(
+    'should not transform the  CSS :active pseudo class Selectors to lowercase',
+    'a:ACTIVE{border: 1px solid black;}',
+    'a:ACTIVE{border: 1px solid black;}'


Why we do not lowercased?

alexander-akait · 2020-04-28T10:45:02Z

packages/postcss-lowercase-text/src/__tests__/index.test.js

+  );
+
+  run(
+    'should safely transform the  CSS nested classname  Selectors to lowercase',


Bad name test

alexander-akait · 2020-04-28T10:45:24Z

packages/postcss-lowercase-text/src/__tests__/index.test.js

+describe('transforming CSS units', () => {
+  run(
+    'should safely transform the absolute units to lowercase : px',
+    'classname{border: 1PX solid black;}',


change classname to .classname

alexander-akait · 2020-04-28T10:46:50Z

packages/postcss-lowercase-text/src/__tests__/index.test.js

+  run(
+    'should safely transform Property ',
+    ':root {  --main-BG-color: coral;} .classname {   color : RED;  background-color: var(--main-BG-color);}',
+    ':root {  --main-BG-color: coral;} .classname {   color : red;  background-color: var(--main-BG-color);}'


Please add test - background-color: VAR(--MY-VAR, VAR(--MY-BACKGROUND, PING));

should PING be lowercased too ?

alexander-akait · 2020-04-28T10:48:19Z

packages/postcss-lowercase-text/src/index.js

+    let allAtRulesNames = []; // ignored list for transforming values
+    css.walkAtRules((rule) => {
+      const allowedTransformingTypes = Object.keys(atRulesTranformerMap);
+      const allowedTransformingTypeSet = new Set(allowedTransformingTypes);


No need initialize inside walk, it is reduce perf

alexander-akait · 2020-04-28T10:48:55Z

packages/postcss-lowercase-text/src/index.js

+          decl.prop = decl.prop.toLowerCase();
+        }
+        // font-family is case sensitive prop, its value should be left as it is
+        if (decl.prop === 'font-family') {


We have many properties which we should lowercased

can you name them ? may all font as well ?

alexander-akait · 2020-04-28T10:49:25Z

packages/postcss-lowercase-text/src/index.js

+        decl.value = valueParser(decl.value)
+          .walk((node) => {
+            i += 1;
+            if (node.type === 'word' && i === 1) {


Avoid using counter, need rewrite

anikethsaha requested a review from alexander-akait January 16, 2020 06:01

anikethsaha mentioned this pull request Jan 28, 2020

mergeLonghand lowercases values with multiple var references #847

Closed

alexander-akait reviewed Jan 28, 2020

View reviewed changes

packages/postcss-lowercase-text/src/__tests__/data/pseudo-elements.js Outdated Show resolved Hide resolved

alexander-akait reviewed Jan 28, 2020

View reviewed changes

packages/postcss-lowercase-text/src/__tests__/atRules.test.js Outdated Show resolved Hide resolved

alexander-akait reviewed Jan 28, 2020

View reviewed changes

packages/postcss-lowercase-text/package.json Outdated Show resolved Hide resolved

anikethsaha mentioned this pull request Jan 28, 2020

[Breaking]: dropping node 6.9 to >=10.13.0 #871

Merged

alexander-akait reviewed Jan 30, 2020

View reviewed changes

packages/cssnano-preset-default/package.json Outdated Show resolved Hide resolved

alexander-akait reviewed Jan 30, 2020

View reviewed changes

packages/postcss-lowercase-text/README.md Outdated Show resolved Hide resolved

alexander-akait reviewed Jan 30, 2020

View reviewed changes

alexander-akait reviewed Feb 4, 2020

View reviewed changes

packages/postcss-lowercase-text/src/__tests__/atRules.test.js Show resolved Hide resolved

alexander-akait reviewed Feb 4, 2020

View reviewed changes

packages/postcss-lowercase-text/src/__tests__/atRules.test.js Outdated Show resolved Hide resolved

alexander-akait reviewed Feb 4, 2020

View reviewed changes

anikethsaha force-pushed the feat-postcss-lowercase-text branch from 57998c5 to b1b0698 Compare April 16, 2020 06:53

feat: postcsss-lowercase-text to default preset

af8a940

anikethsaha force-pushed the feat-postcss-lowercase-text branch from b6a1db5 to af8a940 Compare April 27, 2020 05:44

anikethsaha added 2 commits April 27, 2020 12:52

Merge branch 'master' into feat-postcss-lowercase-text

5513454

chore: updated babel/preset-env to 7.9.x

f8d877d

anikethsaha requested a review from alexander-akait April 27, 2020 08:13

alexander-akait reviewed Apr 28, 2020

View reviewed changes

chore: refactoring in lowercase plugin

da03b82

		@@ -1,5 +1,7 @@
		import processCss from './_processCss';

		jest.setTimeout(30000);

Feat: postcss lowercase text #867

Are you sure you want to change the base?

Feat: postcss lowercase text #867

Conversation

anikethsaha commented Jan 16, 2020 • edited

codecov-io commented Jan 16, 2020 • edited

Codecov Report

anikethsaha commented Jan 28, 2020

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anikethsaha commented Feb 4, 2020 • edited

alexander-akait commented Mar 24, 2020

anikethsaha commented Mar 24, 2020

anikethsaha commented Apr 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anikethsaha commented Jan 16, 2020 •

edited

codecov-io commented Jan 16, 2020 •

edited

anikethsaha commented Feb 4, 2020 •

edited