Browser prefix is operator #45

donotknow · 2018-02-17T01:56:27Z

Which issue # if any, does this resolve?
#38

Please check one:

New tests created for this change
Tests updated for this change

More information on my comment in the original issue #38 (comment).

Update lesshint to work with shellscape/postcss-values-parser#45, which will return a number for +5 instead of an operator and a number. So, check both operators and numbers, ignore any numbers that don’t start with an operator (+-*/), allow the node to be the last node (no nextElement), and use a whitespace regex to check the none spacing instead of the exact value.

…llscape#38)

shellscape · 2018-02-19T03:45:05Z

lib/parser.js

-        if ((!this.current.nodes.length || (this.current.last && this.current.last.type === 'operator')) && this.nextToken[0] === 'word') {
-          return this.word();
-        }
+      if (this.nextToken[0] === 'word') {


so why don't we want this parsed different in loose mode?

Loose mode just means you aren’t compliant CSS, right? It doesn’t mean that this:

https://github.com/shellscape/postcss-values-parser/pull/45/files#diff-cc37320af8aeb8b7582dad90cc114e55R219

-16px -1px -1px 16px

…should be parsed as 3 operators and 4 numbers, it should be four numbers.

But, I mostly leave that question to you as the maintainer. Did I misunderstand that test?

@jwilsson please weigh in here. I recall that we implemented loose mode specifically for less and scss. Will removing how this is processed in loose mode have negative consequences?

I have a matching PR to update less here: lesshint/lesshint#458 (which will need to be updated to bump versions pending a release of this repo). I didn’t realize this was the parser for other things, @jwilsson LMK what else might need to be updated in the same manner as lesshint, I’d prefer to rig up updates to all the repos as part of this review.

Good to know. This might require a major version, as it's going to break some expected functionality.

Yeah, the idea behind loose mode was to target CSS-like languages (such as Less/SCSS).

I definitely think we should tag a major after this one is merged, I know that at least Prettier is using this library (and that's a pretty large project) so we probably wouldn't want to change the behaviour unexpectedly.

Lemme see if I can loop those folks into this. I'll have to comb through the commits to figure out who that was, but we'll get this in once we get some more eyes on it.

@evilebottnawi can you comment as to whether or not this will impact prettier?

alexander-akait · 2018-02-27T18:07:47Z

test/number.js

@@ -209,15 +209,15 @@ describe('Parser → Number : Loose', () => {
    },
    {
      test: '5+5',
-      expected: { value: '5', unit: '', length: 3 }
+      expected: { value: '+5', unit: '', length: 2 }


@shellscape break, 5+5 should be as 5 + 5

@evilebottnawi thanks for taking a look. can you elaborate? I'm not sure what you mean there.

@shellscape It is difficult to say for sure, since we have rather complicated logic around +, - and
/. I will test this branch tomorrow and leave feedback here 👍

alexander-akait · 2018-02-27T19:37:32Z

@shellscape also if it is major change, let's open issue for discussion before release. Some improvement for prettier will be great 👍

shellscape · 2018-02-27T19:42:06Z

@evilebottnawi indeed. this would be a major version change. please do create an issue for discussing what you would like to see improved!

alexander-akait · 2018-02-28T17:13:55Z

@shellscape
Break:

calc (should be ignore, by spec +/- should have space, but if somebody write incorrectly we should output as is):

-   prop75: calc(100%+2px) calc(100% +2px) calc(100%+ 2px) calc(100% + 2px);
-   prop76: calc(100%-2px) calc(100% -2px) calc(100%- 2px) calc(100% - 2px);
+   prop75: calc(100% +2px) calc(100% +2px) calc(100%+ 2px) calc(100% + 2px);
+   prop76: calc(100% -2px) calc(100% -2px) calc(100%- 2px) calc(100% - 2px);

We use parser for some at-rules (example @mixin $(theme)-color). But can/should be fixed inside prettier.

-   @mixin $(theme)-colors;
+   @mixin $(theme) -colors;

Handle scss/less syntax around math operations:

a {
  prop57: round(1.5)+2 round(1.5) +2 round(1.5)+ 2 round(1.5) + 2;
  prop58: 2+round(1.5) 2 +round(1.5) 2+ round(1.5) 2 + round(1.5);
  prop59: (round(1.5)+2) (round(1.5) +2) (round(1.5)+ 2) (round(1.5) + 2);
  prop60: (2+round(1.5)) (2 +round(1.5)) (2+ round(1.5)) (2 + round(1.5));
  prop61: $width+2 $width +2 $width+ 2 $width + 2;
  prop62: 2+$width 2 +$width 2+ $width 2 + $width;
  prop63: ($width+2) ($width +2) ($width+ 2) ($width + 2);
  prop64: (2+$width) (2 +$width) (2+ $width) (2 + $width);
  prop65: @width+2 @width +2 @width+ 2 @width + 2;
}

-   prop57: round(1.5) + 2 round(1.5) + 2 round(1.5) + 2 round(1.5) + 2;
-   prop58: 2 + round(1.5) 2 + round(1.5) 2 + round(1.5) 2 + round(1.5);
-   prop59: (round(1.5) + 2) (round(1.5) + 2) (round(1.5) + 2) (round(1.5) + 2);
-   prop60: (2 + round(1.5)) (2 + round(1.5)) (2 + round(1.5)) (2 + round(1.5));
-   prop61: $width + 2 $width + 2 $width + 2 $width + 2;
-   prop62: 2 + $width 2 + $width 2 + $width 2 + $width;
-   prop63: ($width + 2) ($width + 2) ($width + 2) ($width + 2);
-   prop64: (2 + $width) (2 + $width) (2 + $width) (2 + $width);
-   prop65: @width+2 @width + 2 @width+ 2 @width + 2;
+   prop57: round(1.5) +2 round(1.5) +2 round(1.5) + 2 round(1.5) + 2;
+   prop58: 2 +round(1.5) 2 +round(1.5) 2 + round(1.5) 2 + round(1.5);
+   prop59: (round(1.5) +2) (round(1.5) +2) (round(1.5) + 2) (round(1.5) + 2);
+   prop60: (2 +round(1.5)) (2 +round(1.5)) (2 + round(1.5)) (2 + round(1.5));
+   prop61: $width +2 $width +2 $width + 2 $width + 2;
+   prop62: 2 +$width 2 +$width 2 + $width 2 + $width;
+   prop63: ($width +2) ($width +2) ($width + 2) ($width + 2);
+   prop64: (2 +$width) (2 +$width) (2 + $width) (2 + $width);
+   prop65: @width+2 @width +2 @width+ 2 @width + 2;

From spec in short form:

Note: While scaling can be specified without any spaces, like lightness(*150%), adding/subtracting must be done with spaces after the +/-, or else the +/- will be interpreted as part of the number by the CSS parser.

I.e. - some + 2 -> should be interpreted as operator

alexander-akait · 2018-02-28T17:31:39Z

I think we should avoid behavior in this PR in loose mode, to allow reformat + and / for scss/less/postcss-plugins syntax

shellscape · 2018-02-28T17:33:14Z

@evilebottnawi are your comments new requests/feedback in general for this module or do they pertain to this Pull Request?

alexander-akait · 2018-02-28T18:06:07Z

@shellscape to this

shellscape · 2018-03-20T17:03:44Z

@donotknow & @evilebottnawi can you two work through this so we can merge something that doesn't cause issues for other consumers (like prettier) but fixes the issue that lesshint is currently running into?

alexander-akait · 2018-03-20T17:13:40Z

@donotknow can we use this logic only in not loose mode ?

shellscape · 2019-02-02T16:58:13Z

This has been fixed in next and will be released in a beta later today.

add failing test for browser prefix parsed as operator and not word

51077df

…for shellscape#38

donotknow force-pushed the browser-prefix-is-operator branch from 211984e to 58efda2 Compare February 17, 2018 02:16

donotknow mentioned this pull request Feb 17, 2018

Browser prefix is operator lesshint/lesshint#458

Open

donotknow force-pushed the browser-prefix-is-operator branch from 58efda2 to 75e946e Compare February 17, 2018 03:48

fix: browser prefixes as second property parsed as word in loose (she…

75e946e

…llscape#38)

shellscape reviewed Feb 19, 2018

View reviewed changes

alexander-akait reviewed Feb 27, 2018

View reviewed changes

shellscape added t⁷ ⋅ waiting on upstream x⁴ ⋅ hold labels Dec 22, 2018

shellscape closed this Feb 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Browser prefix is operator #45

Browser prefix is operator #45

donotknow commented Feb 17, 2018 •

edited

shellscape Feb 19, 2018

donotknow Feb 20, 2018 •

edited

shellscape Feb 20, 2018

donotknow Feb 20, 2018

shellscape Feb 20, 2018

jwilsson Feb 20, 2018

shellscape Feb 20, 2018

shellscape Feb 27, 2018

alexander-akait Feb 27, 2018

shellscape Feb 27, 2018

alexander-akait Feb 27, 2018

shellscape Feb 27, 2018

alexander-akait commented Feb 27, 2018

shellscape commented Feb 27, 2018

alexander-akait commented Feb 28, 2018 •

edited

alexander-akait commented Feb 28, 2018 •

edited

shellscape commented Feb 28, 2018

alexander-akait commented Feb 28, 2018

shellscape commented Mar 20, 2018

alexander-akait commented Mar 20, 2018

shellscape commented Feb 2, 2019

Browser prefix is operator #45

Browser prefix is operator #45

Conversation

donotknow commented Feb 17, 2018 • edited

Choose a reason for hiding this comment

donotknow Feb 20, 2018 • edited

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

alexander-akait commented Feb 27, 2018

shellscape commented Feb 27, 2018

alexander-akait commented Feb 28, 2018 • edited

alexander-akait commented Feb 28, 2018 • edited

shellscape commented Feb 28, 2018

alexander-akait commented Feb 28, 2018

shellscape commented Mar 20, 2018

alexander-akait commented Mar 20, 2018

shellscape commented Feb 2, 2019

donotknow commented Feb 17, 2018 •

edited

donotknow Feb 20, 2018 •

edited

alexander-akait commented Feb 28, 2018 •

edited

alexander-akait commented Feb 28, 2018 •

edited