Removed dynamic stylesheet and migrated to CSS vars #2854

lmartorella · 2024-11-14T14:23:46Z

Description

The proposed change is to remove the dynamic CSS classes generation for rows and margins.
This has many benefits: it will speed up relayout operations, optimize performances for high row count and and fix the CSP style compatibility.

The requirement is CSS variables, that is now widely available: https://caniuse.com/css-variables

This deprecates the styleInHead properties as well.

It overrides #2852 as well.

Checklist

Created tests which fail without the change (if possible)
All tests passing (yarn test) (well, the failing test count is not increasing after the change, the main trunk has many failures)
Extended the README / documentation, if necessary

adumesny

I really like the idea of using variables to limit changes, but having 2 styles per items now (we can optimized when 1x1 items like I did) makes me wonder if this will indeed be faster for rendering.

I've wanted to do the same for columns as we only generate CSSS for 1 & 12 and many people forget to include 2-11 and wonder why things don't render. So I would like to do this for column more so actually.

this means complete removal of gs-x,y,w,h as those are only used to refer to classes anyway. We could still support READING them (for backward support), but always clear them during writting so we only have style then.

src/gridstack.scss

src/gridstack.ts

adumesny · 2024-11-14T17:39:17Z

src/gridstack.ts

@@ -2306,7 +2260,7 @@ export class GridStack {
          this.resizeToContentCheck(false, node);
          if (subGrid) {
            subGrid.parentGridNode = node;
-            if (!subGrid.opts.styleInHead) subGrid._updateStyles(true); // re-create sub-grid styles now that we've moved
+            subGrid._updateStyles(); // re-create sub-grid styles now that we've moved


might not be needed. all _updateStyles() need to be verified if needed to be called actually... sicne the external file isn't used now.

Well, my changes are quite conservative, I don't have the full knowledge about the side effects expected by this call (i.e. _updateContainerHeight)

fair enough. but if there was a condition styleInHead then that means it's location based and therefore entire thing doesn't apply.

src/gridstack.ts

adiessl · 2024-11-14T18:29:17Z

I am putting this here as you mentioned it in your comment above @adumesny: we are for example using the attributes gs-x, gs-y. gs-h and gs-w to style grid cells based on their dimensions using purely CSS. Should these no longer exist, it would be much more difficult to achieve the same dynamic styling as it would involve additional code to do so.

It should be no problem, however, if they were replaced with unitless CSS custom properties like --gs-h and so on. If they are not unitless, say --gs-h would be 100px or 33%, it would not work. So please keep in mind that there might be people using these attributes and removing them might cause breaking changes. Thank you.

adumesny · 2024-11-14T19:12:59Z

@adiessl how would unitless CSS work for you ? wouldn't all have --gs-h then and would would you tell them appart (height of 1 vs 2) ?
so you are saying you style a cell h=1 and h=2 (or width) differently ? Maybe it could be accomplished using custom class per types instead, or key off class.height value it will become ?

adiessl · 2024-11-15T08:58:18Z

I forked the official Fiddle and amended the CSS to show how we are using the attributes at the moment to display the grid cells' dimensions: https://jsfiddle.net/2y7w3jL0/

Unfortunately I was mistaken about being able to use unitless CSS custom properties. A CSS custom property cannot be used as content of an after element.

adumesny · 2024-11-15T15:29:43Z

@adiessl cool example (missing 1x1 case when no attr). we should add that as a demo.

adumesny

do you want to do column as well to match ?

adumesny · 2024-11-15T15:50:56Z

spec/gridstack-spec.ts

@@ -1370,41 +1370,24 @@ describe('gridstack', function() {
    afterEach(function() {
      document.body.removeChild(document.getElementById('gs-cont'));
    });
-    it('should add STYLE to parent node as a default', function() {
-      var options = {
+    it('should not add STYLE to parent node', function() {


just remove - obsolete code.

spec/gridstack-spec.ts

src/gridstack.scss

adumesny · 2024-11-15T16:02:09Z

src/gridstack.scss

@@ -98,6 +100,19 @@ $animation_speed: .3s !default;
  > .ui-resizable-sw { cursor: sw-resize; width: 20px; height: 20px;}
  > .ui-resizable-w  { cursor: w-resize;  width: 10px; top: 15px; bottom: 15px; }

+  > .ui-resizable-n {
+    top: var(--gs-item-margin-top);


that conflicts with 95. need to figure out why I have 0 while you have '5' - either way we don't want 2 rules...
same with rest below...

This comes from the old code:

Utils.addCSSRule(this._styles, `${prefix} > .ui-resizable-n`, `top: ${top};`);

yep ok, so replace my 0 with your var then (just one css rule). all your resize should merge into my existing rules.

src/gridstack.scss

adumesny · 2024-11-15T16:11:12Z

src/gridstack.ts

@@ -1317,7 +1308,7 @@ export class GridStack {
          // restore any sub-grid back
          if (n.subGrid?.el) {
            itemContent.appendChild(n.subGrid.el);
-            if (!n.subGrid.opts.styleInHead) n.subGrid._updateStyles(true); // force create
+            n.subGrid._updateStyles();


again if we skipped redoing styles that were global, then most likely we don't need to do anything... need to go though all calls and only call when what we upate changes...
ONLY when cellHeigt OR margin is changed, OR number of rows for grid total height (might need to tweak that)

Same as above, since my changes are quite conservative, I don't feel that the performances are hurting so much in removing the line. Probably better to split _updateStyles and extract _updateContainerHeight from it?

src/gridstack.ts

adumesny · 2024-11-15T16:47:04Z

src/gridstack.ts

@@ -1635,7 +1635,12 @@ export class GridStack {
    if ((!n._moving && !n._resizing) || this._placeholder === el) {
      // Set inline style, refer CSS variables
      el.style.top = `calc(${n.y} * var(--gs-cell-height))`;
-      el.style.height = `calc(${n.h} * var(--gs-cell-height))`;
+      if (n.h !== 1) {


n.h > 1 ? el.style.height = calc(${n.h} * var(--gs-cell-height)) : delete el.style.height; // default h=1 set globally

to match above case.

This will trigger the

Extract the assignment of "el.style.height" from this expression.sonarlint(typescript:S1121)

sonar lint issue, part of the recommended TS set.
I quite agree to that, being a "delete" part of else branch as well.
A better way is probably trying to assign an empty string to height, or undefined.

adumesny · 2024-11-18T22:52:54Z

changes in #2874 need to be cherry picked too.

- Removed dynamic stylesheet and migrated to CSS vars

bfa84fa

lmartorella mentioned this pull request Nov 14, 2024

Support gridstack element node reparenting without losing the stylesheets #2852

Open

3 tasks

lmartorella added 2 commits November 14, 2024 15:45

- Fixed flickering during resize

cf62b1d

- Fixed placeholder not moving vertically

bf7a96a

adumesny reviewed Nov 14, 2024

View reviewed changes

lmartorella added 5 commits November 15, 2024 12:09

- Merged SCSS definitions

aab0fb2

- Tied CSS vars to gridstack element rather than the parent

9cb7ef5

- Removed unused code

db2615e

- Reverted n renamed to node

6bad283

- Separated drag/resize

b5624d9

adumesny reviewed Nov 15, 2024

View reviewed changes

lmartorella added 3 commits November 15, 2024 17:25

- Removed obsolete tests

60c75bb

- Linted

9b013fa

- Less rule in case of 1x1 elements

57d75a3

adumesny reviewed Nov 15, 2024

View reviewed changes

- Linted

d618ef5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed dynamic stylesheet and migrated to CSS vars #2854

Removed dynamic stylesheet and migrated to CSS vars #2854

lmartorella commented Nov 14, 2024 •

edited

Loading

adumesny left a comment •

edited

Loading

adumesny Nov 14, 2024

lmartorella Nov 15, 2024

adumesny Nov 15, 2024

adiessl commented Nov 14, 2024

adumesny commented Nov 14, 2024

adiessl commented Nov 15, 2024

adumesny commented Nov 15, 2024

adumesny left a comment

adumesny Nov 15, 2024

adumesny Nov 15, 2024

lmartorella Nov 15, 2024

adumesny Nov 15, 2024

adumesny Nov 15, 2024

lmartorella Nov 15, 2024

adumesny Nov 15, 2024

lmartorella Nov 16, 2024

adumesny commented Nov 18, 2024

Removed dynamic stylesheet and migrated to CSS vars #2854

Are you sure you want to change the base?

Removed dynamic stylesheet and migrated to CSS vars #2854

Conversation

lmartorella commented Nov 14, 2024 • edited Loading

Description

Checklist

adumesny left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiessl commented Nov 14, 2024

adumesny commented Nov 14, 2024

adiessl commented Nov 15, 2024

adumesny commented Nov 15, 2024

adumesny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adumesny commented Nov 18, 2024

lmartorella commented Nov 14, 2024 •

edited

Loading

adumesny left a comment •

edited

Loading