kendo-themes icon indicating copy to clipboard operation
kendo-themes copied to clipboard

Import inside class throws an error

Open JoomFX opened this issue 4 years ago • 10 comments

Describe the bug Kendo Themes cannot be imported inside a class when Dart Sass 2+ is used. The following errors are thrown:

DEPRECATION WARNING: As of Dart Sass 2.0.0, !global assignments won't be able to
declare new variables. Consider adding `$imported-modules: null` at the root of the
stylesheet.

DEPRECATION WARNING: As of Dart Sass 2.0.0, !global assignments won't be able to
declare new variables. Consider adding `$data-uris: null` at the root of the
stylesheet.

To reproduce Try to compile the following code:

.dark-theme {
  $primary-palette-name: yellow;

  @import "~@progress/kendo-theme-material/dist/all.scss";
}

Expected behavior Currently, the issue can be overcome by adding the following two lines at the top of you stylesheet:

$imported-modules: ();
$data-uris: ();

But this should be fixed in the themes. The code that needs to be looked at is located here and here.

Affected package (please remove the unneeded items)

  • theme-default
  • theme-bootstrap
  • theme-material

Affected suites (please remove the unneeded items)

  • Kendo UI for jQuery
  • Kendo UI for Angular
  • Kendo UI for React
  • Kendo UI for Vue
  • Telerik UI for Blazor

Affected browsers (please remove the unneeded items)

  • All

Build system information (please remove the unneeded items)

  • Not Applicable

JoomFX avatar Feb 08 '21 16:02 JoomFX

👀 Any update on prioritizing this one? Same as described 👆

ianwesterfield avatar May 28 '21 18:05 ianwesterfield

@ianwesterfield The issue is related to how variables are treated when nested inside classes.

To go into details, we convert non text files (images, fonts) to base64 encoded string and append them in an aptly named variable called $data-uris. Later, we retrieve the content by key.

When you are using the theme as is, everything is fine and the variable is found.

However when you nest the import things get a bit strange, for a lack of a better word. I am not sure for the exact reason, but in this nested import scenario, when we try to append said encoded content it's not appended in the same location where we try to retrieve it from.

Good news is that this all can be workaround relatively easy: we just need to reset one variable before each such import. Resetting the variable will make it globally available.

Also, because there is not true "import once" or something similar in sass and because we've optimized for one theme per file, we do a bit of trickery to ensure content is loaded once and only once. It's a very rudimentary caching where we cache modules by keys. So you need to modify another variable.

There might be an additional issue with having @font-face directive inside code that can be imported in a class... While I am definitely sure this is a bug in sass, it's unlikely the issue will be fixed in node-sass.

Overall, the issue falls somewhere in the middle of shady practices and sass itself. I am pretty confident that if we asked the sass team "why" we would get "you brought it on yourself" type of answer.

Still, the issue is very easy to fix:

$imported-modules: (); // reset the list of loaded modules for good measure
$data-uris: (); // reset the data-uris thus making them globally available

.dark-theme {
    @import "all.scss";
}


$imported-modules: ();
$data-uris: ();

.light-theme {
    @import "all.scss";
}

If you got the patience to read this far, I am sensing a question coming my way. Somewhere in the lines of "why should we do that? why don't you fix your themes instead?", which is an excellent question. I did mentioned that we optimized for "caching" and we also optimized for "dependencies" -- meaning we link files in such a way that if, say, someone imports the grid styles, we'll also import styles for pager, button, various inputs that can be in the grid.

So it's not a matter of "fixing" but a complete architectural rewrite of our theme framework. Which we'll do and when we do so, it's going to be a major version of the themes.

joneff avatar Jun 09 '21 11:06 joneff

Parts of the issue can be mitigated by wrapping @font-face in @at-root... Seem sketchy, but does the job.

joneff avatar Jul 02 '21 09:07 joneff

Hi @joneff . I tried to follow your approach for 6 ro so themes in my recent project and by doing so i wanted to ask: did you also experienced the increase in compiled styles.css in /dist. i just did it for scheduler and splitter from kendo and it had a size factor of *2.2 or so. Just asking and greeting.

janpauldahlke avatar Nov 16 '21 19:11 janpauldahlke

If possible, what is the recommended way of doing this with the latest version (6.4.0)?

Something like this, in combination with bootstrap 5.3 would be awesome:

html[data-bs-theme=light] {
    @import "~@progress/kendo-theme-bootstrap/dist/bootstrap-main";
}

html[data-bs-theme=dark] {
    @import "~@progress/kendo-theme-bootstrap/dist/bootstrap-main-dark";
}

This results in a bunch of errors and warnings.

Other suggestions for easy color mode switching are also welcome 🙂

palhal avatar Jun 19 '23 10:06 palhal

In Kendo Themes v6.4.0 the correct syntax is this:

$_imported: (); // reset the list of imported modules by the new modules system making them globally available
$_kendo-imported-modules: (); // reset the list of loaded modules for good measure
$_kendo-data-uris: (); // reset the data-uris thus making them globally available

.dark-theme {
    @import '@progress/kendo-theme-default/dist/all.scss';
}

$_imported: ();
$_kendo-imported-modules: ();
$_kendo-data-uris: ();

.light-theme {
    @import '@progress/kendo-theme-default/dist/all.scss';
}

The first line is needed because of the new module system that we introduced in Kendo Themes v6.3.0. The other two lines were always needed when importing the theme inside a class. We just renamed the SCSS variables (added the kendo prefix).

JoomFX avatar Jun 19 '23 10:06 JoomFX

Thank you for the quick reply and thorough explanation. Your example almost worked as-is with the default theme, but I got an error from sass-loader: Function not found: k-try-shade. I resolved that by adding this at the very top:

@import '@progress/kendo-theme-core/dist/all';

When replacing default with bootstrap, I got lots of Found no color leading to 7:1 contrast ratio against #xxxxxx...

I resolved that by adding the following above the import already mentioned. Related to #4284?

$wcag-min-contrast-ratio: 4.5 !default;

I took 4.5 from all.scss, but I see that the same variable is defined as 7 further down in the same file which makes me doubt whether this is the correct value. For all I know there could be other variables that also should be defined.

I am far from a sass expert and I struggle a bit with getting an overview of all these nested include files. Is this a suitable approach? Your help is greatly appreciated.

palhal avatar Jun 19 '23 21:06 palhal

I wasn't able to reproduce the issues you described, however I got another error when trying to compile our Kendo Bootstrap theme inside a class.

When I tried to compile our Kendo Default theme inside a class I got no issues at all. When I tried to compile our Kendo Bootstrap theme inside a class I got the following error:

SassError: $color2: 1 is not a color.
    ╷
101 │     @return mix( $color1, $color2, $weight );
    │             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵
  node_modules\@progress\kendo-theme-bootstrap\dist\all.scss 101:13    k-color-mix()
  node_modules\@progress\kendo-theme-bootstrap\dist\all.scss 59580:23  @import
  src\styles.scss 8:13  

The issue comes from the DataViz variables. For some reason the k-color-mix function cannot interpolate the variables properly when the theme is being imported inside a class. The workaround is to assign the variables the actual values like this:

$kendo-series-a: #0d6efd !default;
$kendo-series-b: #6f42c1 !default;
$kendo-series-c: #20c997 !default;
$kendo-series-d: #198754 !default;
$kendo-series-e: #ffc107 !default;
$kendo-series-f: #dc3545 !default;

I am attaching a sample Angular app where you can see the above workaround in action. You can also try to import the Default theme (instead of Bootstrap).

test-themes.zip

Regarding the issues you experience - I'm not really sure why you get these errors. It is probably a conflict that comes from other imports. But it is not really possible to say anything without being able to reproduce the issue on our end.

Yes, importing the themes inside a class (scoping the CSS to a class) is a suitable approach. Your workarounds (importing the Core package and setting the variable) looks good to me, even though you shouldn't be getting these errors.

If you want your issues to be looked at and investigated further, it will be best if you open a support ticket so we can collect all the needed data and troubleshoot it.

JoomFX avatar Jun 20 '23 12:06 JoomFX

Thanks again. I'm using webpack with sass-loader, maybe that's why we don't have the same issues. Anyway, I'm very happy that it finally works. I have spent many hours on this, so I will post my end result which hopefully can help others. Here's the absolute minimum I needed to get it working:

$wcag-min-contrast-ratio: 4.5 !default;

@import "@progress/kendo-theme-core/dist/all.scss";

$_imported: (); // reset the list of imported modules by the new modules system making them globally available
$_kendo-imported-modules: (); // reset the list of loaded modules for good measure
$_kendo-data-uris: (); // reset the data-uris thus making them globally available

.light-theme {
    @import "@progress/kendo-theme-bootstrap/dist/bootstrap-main.scss";
}

$_imported: ();
$_kendo-imported-modules: ();
$_kendo-data-uris: ();

.dark-theme {
    @import "@progress/kendo-theme-bootstrap/dist/bootstrap-main-dark.scss";
}

The double reset isn't required to build, but without it the second imported theme doesn't work at runtime. This applies to both default and bootstrap. Setting the DataViz variables are not necessary since I'm importing bootstrap-main[-dark] instead of all.

palhal avatar Jun 20 '23 15:06 palhal

I'm glad you figured it out and thank you for posting your findings. They will surely be helpful for others. Yep, my bad, I confirm that the second reset is needed. I updated my previous comment.

JoomFX avatar Jun 21 '23 07:06 JoomFX