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

[BUG] node-sass does not properly compile nested media queries #959

Closed
dheineman opened this issue Dec 10, 2021 · 5 comments
Closed

[BUG] node-sass does not properly compile nested media queries #959

dheineman opened this issue Dec 10, 2021 · 5 comments
Labels
bug Something isn't working

Comments

@dheineman
Copy link
Contributor

dheineman commented Dec 10, 2021

Describe the bug

It seems that the fix a9e1336 @rjd22 commited to solve #892 does not work properly with node-sass as this does not seem to compile the nested media query correctly thus leaving it out of the generated css entirely.

I was able to reproduce this behaviour with the tabler repo itself using the steps below. But an easier way to confirm this is just by searching for theme-dark-auto in v1.0.0-beta5/dist/css/tabler.css as compared to v1.0.0-beta4

The issue can be solved by using sass instead of node-sass (which does require #956 to be merged first) and given that node-sass itself is deprecated this might be the best solution.

From what i can tell there are minimal behavioral differences for tabler between the two versions (namely the numeric precision seems to increase by 1) but some further testing might be required.

To reproduce

Steps to reproduce the behavior:

  1. Checkout a the main branch of tabler
  2. Apply the patch from Update _mixins.scss #956
  3. run npm run build

You can see that currently the only changes in dist/css/tabler.css are from pull request 956. And that the file does not contain any reference to the theme-dark-auto class.

  1. Switch to using sass by applying the following changes.
diff --git a/gulpfile.js b/gulpfile.js
index accbd6cc..26f08b7d 100644
--- a/gulpfile.js
+++ b/gulpfile.js
@@ -1,7 +1,7 @@
 const gulp = require('gulp'),
 	debug = require('gulp-debug'),
 	clean = require('gulp-clean'),
-	sass = require('gulp-sass')(require('node-sass')),
+	sass = require('gulp-sass')(require('sass')),
 	postcss = require('gulp-postcss'),
 	header = require('gulp-header'),
 	cleanCSS = require('gulp-clean-css'),
diff --git a/package.json b/package.json
index dece16f3..94fdc77b 100644
--- a/package.json
+++ b/package.json
@@ -89,7 +89,7 @@
     "gulp-sass": "^5.0.0",
     "imask": "^6.2.2",
     "litepicker": "^2.0.11",
-    "node-sass": "^6.0.1",
+    "sass": "^1.44.0",
     "nouislider": "^15.5.0",
     "postcss": "^8.3.11",
     "release-it": "^14.11.6",
  1. Run npm install to install the different sass version.
  2. And do another npm run build to rebuild the css.

Now the dist/css/tabler.css has a lot of changes but these are mostly related to code styling. The more interesting thing is that the file now does contain thetheme-dark-auto class required for auto dark mode to work.

@dheineman dheineman added the bug Something isn't working label Dec 10, 2021
@rjd22
Copy link
Collaborator

rjd22 commented Dec 11, 2021

I've been using tabler in combination with the sass library without the mixins fix for a while now without any issues.

Maybe an older version of sass fixes the issue till the mixins fix is released?

@dheineman
Copy link
Contributor Author

An older version might fix it. Not sure what version of sass you are using but the latest (1.44.0) gave me errors without the mixins fix.

But the mixins pull request only fixes building with sass. Tabler itself uses node-sass. Which means that the theme-dark-auto css code or any future nested media query will not be present in the dist files which is what this issue is really about.

@rjd22
Copy link
Collaborator

rjd22 commented Dec 13, 2021

@dheineman Ah, I will fix that next tuesday, but if you make an PR I will gladly merge it today.

@codecalm
Copy link
Member

@dheineman I've made PR for this change: #963

chan you check it?

@dheineman
Copy link
Contributor Author

Look good. thanks @codecalm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants