GetmeUK / ContentTools

A JS library for building WYSIWYG editors for HTML content.
http://getcontenttools.com
MIT License
3.94k stars 393 forks source link

Support for Sass >= 1.33 #576

Closed Ei-aaie closed 2 years ago

Ei-aaie commented 2 years ago

Hello, since sass 1.33.0 (dart-sass), they introduced deprecation warning for division operation with slash.

See : https://sass-lang.com/documentation/breaking-changes/slash-div

I use ContentTools in one of my project, and got a lot of warnings that I can't fix :)

2 options : using css calc()or @use 'sass:math';

Thanks !

anthonyjb commented 2 years ago

@Ei-aaie thanks for the heads up on this - there's not a lot of CSS in the project and SASS provide a nice migration tool by the looks of it so I'll push a fix up this weekend.

Ei-aaie commented 2 years ago

I forked the project, it looks like it runs a Ruby sass version, not Dart-sass

The result is that the migration does the job, but the build fails at : sass:build

Error: Invalid CSS after "...  $number: math": expected "}", was ".div($number, $..."
        on line 45 of external/styles/bourbon/helpers/_str-to-num.scss
        from line 25 of external/styles/bourbon/_bourbon.scss
        from line 5 of src/styles/content-tools.scss

40:       }
41: 
42:       @else {
43:         // Move the decimal dot to the left
44:         $divider: $divider * 10;
45:         $number: math.div($number, $divider);
46:       }
47: 
48:       $result: $result + $number;
49:     }
50:   }

sass:mathlib is dart-sass specific :) There is a dart-sass grunt package, but I'm not very familiar with grunt / coffee synthax https://www.npmjs.com/package/grunt-dart-sass

anthonyjb commented 2 years ago

So I think locally now we use libsass for comping the project, but there's probably still the old ruby sass requirements in the last release of CT. Will look into what's possible, it's likely some of if not all the divisions can be removed to be honest.

Ei-aaie commented 2 years ago

After compiling locally, ~50% can be replaced by multiplication without changing any deps, for example :

-        margin-top: -($dialogHeight / 2);
+        margin-left: -($dialogWidth * 0.5);

sass-migrator uses this kind of smart fix if t's possible, and if not, applies math.div

anthonyjb commented 2 years ago

@Ei-aaie thank you for the report and your work looking into the issue - I've pushed up a new release 1.6.15 which removes the reliance on the division operator I believe.

Multiplication would have been a good way to go on thinking about it but in the end I just when we declaring half sizes where convenient.

Let me know if you are still get any warnings in dart-scss now.

Ei-aaie commented 2 years ago

@anthonyjb It's almost it ! There are only 2 warnings left : /external/styles/bourbon/functions/_strip-units.scss 4:12

and /external/styles/bourbon/functions/_px-to-em.scss 12:12

anthonyjb commented 2 years ago

Ahh that's an external library not part of ct - I'll see if I can remove ideally or upgrade if not.

anthonyjb commented 2 years ago

@Ei-aaie if you look at the latest release of bourbon they don't support Dart SASS yet and had to revert the fix for div (https://github.com/thoughtbot/bourbon/issues/1106#issuecomment-1048061338). So for now I'm just going to patch the version I include in CT if I can as they form part of the repo within the external directory. If that doesn't work I'll remove the dependency (but looking at it that's a bigger job so would be another weekend away).

anthonyjb commented 2 years ago

@Ei-aaie ok can we try again, new release 1.6.16 - let me know.

Ei-aaie commented 2 years ago

Working like a charm ! Thank you !

anthonyjb commented 2 years ago

@Ei-aaie great thank you for the help with issue.