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

Deprecation warnings after upgrading Sass when using ms Variable #171

Open
joyheron opened this issue May 25, 2021 · 21 comments
Open

Deprecation warnings after upgrading Sass when using ms Variable #171

joyheron opened this issue May 25, 2021 · 21 comments

Comments

@joyheron
Copy link

I just upgraded my Sass version to 1.33.0, but now I have a lot of deprecated messages whenever I use the ms function within my code base. Since I use it in many different places in my code base, this occurs quite often. Is there any chance you could update your package so that these messages disappear?

DEPRECATION WARNING: Using / for division is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div($ms-return, $b)

More info and automated migrator: https://sass-lang.com/d/slash-div

   ╷
35 │       $ms-return: $ms-return / $b;
   │                   ^^^^^^^^^^^^^^^
   ╵
    node_modules/modularscale-sass/stylesheets/modularscale/_pow.scss 35:19       ms-pow()
    node_modules/modularscale-sass/stylesheets/modularscale/_function.scss 15:13  ms-function()
    node_modules/modularscale-sass/stylesheets/modularscale/_sugar.scss 6:11      ms()
    lib/styles/base/_spacing.scss 10:15                                           @import
    lib/styles/base/index.scss 3:9                                                @import
    components/05-atoms/colors/index.scss 1:9                                     root stylesheet
@scottkellum
Copy link
Member

Thank you. I have not updated in a while and Sass functionality as well as CSS has come a long way over the years. It may be about time for a re-write to take advantage of the new Sass module system. My main focus has been another typography tool, Typetura, and it's been a while since I've done modular scale stuff.

@spenno
Copy link

spenno commented May 26, 2021

@joyheron: The same thing happened to me yesterday. Until this is updated, I have copied the stylesheets/_modularscale.scss Sass file and the stylesheets/modularscale folder into a vendors folder in my local source files and imported that version of the Sass file instead of the one in node_modules.

Then I updated the math divisions in three of the Sass partials to use Sass 1.33's new maths.div function.

You can see the changes I made here: https://github.com/modularscale/modularscale-sass/compare/3.x...spenno:feature/math-division?expand=1

And a quick summary of the changes made:

_functions.scss line 29: $ms-base: math.div($ms-base, $ratio);
_pow.scss line 35: $ms-return: math.div($ms-return, $b);
_strip-units.scss line 6: @return (math.div($val, ($val - $val + 1)));

I would submit a pull request for this @scottkellum, but it would completely break for people still using LibSass. Although LibSass is now deprecated, I'm guessing you would prefer a more elegant transition than that :)

@scottkellum
Copy link
Member

@spenno Breaking changes are what Semantic Versioning and dependency lists are for. I may go ahead and do a major version bump.

Now that exponents are now native to the Sass that will simplify a ton of code. Might as well do a big re-write.

Note that I’ll likely make some changes to ms-respond in the process. Changes there may be breaking with that mixin, but I’ll try to avoid it. There have been a lot of advancements here and plenty of better ways to implement responsive typography.

@joyheron
Copy link
Author

Thank you for looking into this!

@strarsis
Copy link

Related to exponents in sass: #113

@scottkellum
Copy link
Member

Status update: The thing holding this up now is the responsive logic

@strarsis
Copy link

Does it make sense to prepare it for the upcoming CSS Container Queries?

@scottkellum
Copy link
Member

@strarsis Yes, part of the reason why I want to update it to output Typetura styles instead of locks/clamp as an option. It would be easier with post-processing stylesheets as opposed to pre-processing as you have to be more explicit in pre-processing. Use cases like #148 are difficult to build a nice syntax around.

@scottkellum
Copy link
Member

I got things working and the syntax is going to be something like this:

@include ms.step using ($respond) {
  foo {
    // This will be responsive
    bar: ms.step(2, $settings: $respond);

    // If for some reason you don’t want something to be responsive in these blocks, you can do this
    baz: ms.step(2)
    
    // Shorthand will work here! And you can mix responsive and non-responsive values.
    padding: ms.step(2, $settings: $respond) ms.step(1)

  }
}

I do not think I will support clamp() or locks. It will either be media queries, container queries, or Typetura.

Related to issue #148

@scottkellum
Copy link
Member

New release you can test out here: https://github.com/modularscale/modularscale-sass/releases/tag/v4.0.0.rc1

@robsonsobral
Copy link
Collaborator

@scottkellum , hi!

I'm sorry, but I didn't get the expose of the ms.$settings map. Why not to use with() and make the settings private?

@use '../vendor/modularscale' with (
  $settings: (base: 1rem, ratio: 1.25),
);

@forward '../vendor/modularscale';

It can be "imported" once and forwarded already configured.

@scottkellum
Copy link
Member

@robsonsobral thanks Rob, I will definitely look into this approach. I do like the ability to change the settings throughout your style sheet as needed, so I might want to make this work with that functionality.

@robsonsobral
Copy link
Collaborator

https://sass-lang.com/documentation/at-rules/forward#configuring-modules

@scottkellum
Copy link
Member

Thanks for your feedback @robsonsobral, I drafted a new release here: https://github.com/modularscale/modularscale-sass/releases/tag/v4.0.0.rc2

@robsonsobral
Copy link
Collaborator

@scottkellum , I've been really busy, but I would like to do some tests before the release, if you don't mind.

@scottkellum
Copy link
Member

@robsonsobral I understand but I am also really busy. I would love a PR here if anyone has the time and is interested in contributing.

If you do contribute please keep any configuration and dependencies to a minimum.

If no one can help I may release next week without tests.

@scottkellum
Copy link
Member

To clarify, @ everyone reading this.

I am looking for help to write tests and ensure distribution channels are solid before release.

@robsonsobral
Copy link
Collaborator

This lib/addon/plugin/toolkit/whatever has been a constant part of my work for years. So, I gonna to give it back to the community.

@gvjacob
Copy link

gvjacob commented Nov 22, 2021

Sharing a fairly low-effort workaround that works well for me. Install sass-migrator and put the following script in your package.json

"postinstall": "sass-migrator division node_modules/modularscale-sass/stylesheets/modularscale/*.scss"

On every npm install, the script will convert any / divisions to math.div in the plugin's output files. Some context to sass-migrator here

@joyheron
Copy link
Author

joyheron commented Jan 4, 2022

@scottkellum Thank you for your work on this!

I have been testing out the new 4.x implementation, and for the most part it seems to work (I made an Issue for the only error that I have: #176 )

One things that I noticed is that the migration from 3.x to 4.x is a bit tricky because a CSS rule like width: ms(2) (which needs to be replaced with ms.step(2)) isn't recognized as an error by the Sass compiler so the error isn't obvious at compile time (since it isn't valid CSS, the rule is ignored and the layout is just a bit (or a lot) off). But other than that, I was successful in migrating my code.

I am very happy to continue testing in the future.

iegik added a commit to iegik/modularscale-sass that referenced this issue Mar 22, 2023
@heun01
Copy link

heun01 commented Jul 20, 2023

Since nothing has happened here for a long time, can we still expect an update?

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

No branches or pull requests

7 participants