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.

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

[styles] Remove deprecated division syntax #5682

Closed
wants to merge 2 commits into from

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Mar 2, 2022

Summary

Removes deprecated and soon to be removed division syntax in scss files. This currently only shows a deprecation warning but when used in storybook throws an error.

fix #4912

I ran the suggested migrator as shown below.

npm install -g sass-migrator
sass-migrator division **/*.scss

This favors replacing division with multiplication of decimals for simple cases such as...

- $height: $euiSizeM / 2;
+ $height: $euiSizeM * .5;

For more complex divisors it converts to using math.div such as...

- $line-height: ($baseLineHeightMultiplier * 6) / $fontSizeH1;
+ @use "sass:math";
+ $line-height: math.div(($baseLineHeightMultiplier * 6) / $fontSizeH1);

But this is not supported in node-sass so instead I convert this to use calc...

- $line-height: ($baseLineHeightMultiplier * 6) / $fontSizeH1;
+ $line-height: calc(($baseLineHeightMultiplier * 6) / $fontSizeH1);

Finally cleanup styles with...

yarn lint-sass-fix

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

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in Chrome, Safari, Edge, and Firefox
  • A changelog entry exists and is marked appropriately

@nickofthyme
Copy link
Contributor Author

Unfortunately using the calc approach in complex functions does not work in place of math.div. In order to fix this eui would need to migrate from node-sass to sass.

@nickofthyme nickofthyme closed this Mar 3, 2022
@nickofthyme nickofthyme deleted the replace-division branch March 3, 2022 17:00
@wapenshaw
Copy link

wapenshaw commented Mar 10, 2022

In order to fix this eui would need to migrate from node-sass to sass.

@nickofthyme
is this going to happen in the future? curently I am getting around this by sticking to sass@1.32.13 which works

@nickofthyme
Copy link
Contributor Author

@wapenshaw not sure it's a priority since they are moving to CSS-in-JS.

@chandlerprall any thoughts on migrating from node-sass to sass? Another alternative to migrating to sass might be to use sass-embedded.

@chandlerprall
Copy link
Contributor

not sure it's a priority since they are moving to CSS-in-JS

Indeed, upgrading (or exchanging) those dependencies is very low priority, and has been, because of the CSS-in-JS effort. It's unlikely anyone from the team would pick that up, but we'd be willing to work with anyone else who wanted to take it on.

Fweddi added a commit to guardian/typerighter that referenced this pull request Jul 13, 2023
>1.32, SASS spits out deprecation warnings for certain calc() functions. These warnings are from our dependency on ElasticUI, who currently have no intention to fix them - see: elastic/eui#5682 (comment). The best solution until ElasticUI have fixed these warnings is to pin our SASS version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove deprecated / sass syntax
3 participants