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.

Sign up for GitHub

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

Removed dynamic stylesheet and migrated to CSS vars #2854

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Removed dynamic stylesheet and migrated to CSS vars #2854

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
bfa84fa
- Removed dynamic stylesheet and migrated to CSS vars
lmartorella Nov 14, 2024
cf62b1d
- Fixed flickering during resize
lmartorella Nov 14, 2024
bf7a96a
- Fixed placeholder not moving vertically
lmartorella Nov 14, 2024
aab0fb2
- Merged SCSS definitions
lmartorella Nov 15, 2024
9cb7ef5
- Tied CSS vars to gridstack element rather than the parent
lmartorella Nov 15, 2024
db2615e
- Removed unused code
lmartorella Nov 15, 2024
6bad283
- Reverted `n` renamed to `node`
lmartorella Nov 15, 2024
b5624d9
- Separated drag/resize
lmartorella Nov 15, 2024
60c75bb
- Removed obsolete tests
lmartorella Nov 15, 2024
9b013fa
- Linted
lmartorella Nov 15, 2024
57d75a3
- Less rule in case of 1x1 elements
lmartorella Nov 15, 2024
d618ef5
- Linted
lmartorella Nov 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion doc/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ GridStack will add it to the `<style>` elements it creates.
- `row` - fix grid number of rows. This is a shortcut of writing `minRow:N, maxRow:N`. (default `0` no constrain)
- `rtl` - if `true` turns grid to RTL. Possible values are `true`, `false`, `'auto'` (default: `'auto'`) See [example](https://gridstackjs.com/demo/right-to-left(rtl).html)
- `staticGrid` - removes drag|drop|resize (default `false`). If `true` widgets are not movable/resizable by the user, but code can still move and oneColumnMode will still work. You can use the smaller gridstack-static.js lib. A CSS class `grid-stack-static` is also added to the container.
- `styleInHead` - if `true` will add style element to `<head>` otherwise will add it to element's parent node (default `false`).

### Responsive
v10.x supports a much richer responsive behavior, you can have breakpoints of width:column, or auto column sizing, where no code is longer needed.
Expand Down
47 changes: 1 addition & 46 deletions spec/gridstack-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,52 +1361,7 @@ describe('gridstack', function() {
let grid = GridStack.init(options);
expect(grid.el.classList.contains('grid-stack-rtl')).toBe(false);
});
});

describe('grid.opts.styleInHead', function() {
beforeEach(function() {
document.body.insertAdjacentHTML('afterbegin', gridstackHTML);
});
afterEach(function() {
document.body.removeChild(document.getElementById('gs-cont'));
});
it('should add STYLE to parent node as a default', function() {
var options = {
cellHeight: 80,
verticalMargin: 10,
float: false,
};
var grid = GridStack.init(options);
expect((grid as any)._styles.ownerNode.parentNode.tagName).toBe('DIV'); // any to access private _styles
});
it('should add STYLE to HEAD if styleInHead === true', function() {
var options = {
cellHeight: 80,
verticalMargin: 10,
float: false,
styleInHead: true
};
var grid = GridStack.init(options);
expect((grid as any)._styles.ownerNode.parentNode.tagName).toBe('HEAD'); // any to access private _styles
});
});

describe('grid.opts.styleInHead', function() {
beforeEach(function() {
document.body.insertAdjacentHTML('afterbegin', gridstackHTML);
});
afterEach(function() {
document.body.removeChild(document.getElementById('gs-cont'));
});
it('should add STYLE to parent node as a default', function() {
var grid = GridStack.init();
expect((grid as any)._styles.ownerNode.parentNode.tagName).toBe('DIV');
});
it('should add STYLE to HEAD if styleInHead === true', function() {
var grid = GridStack.init({styleInHead: true});
expect((grid as any)._styles.ownerNode.parentNode.tagName).toBe('HEAD');
});
});
});

describe('grid.enableMove', function() {
beforeEach(function() {
Expand Down
17 changes: 0 additions & 17 deletions spec/utils-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,23 +48,6 @@ describe('gridstack utils', function() {
});
});

describe('test createStylesheet/removeStylesheet', function() {

it('should create/remove style DOM', function() {
let _id = 'test-123';
Utils.createStylesheet(_id);

let style = document.querySelector('STYLE[gs-style-id=' + _id + ']');
expect(style).not.toBe(null);
// expect(style.prop('tagName')).toEqual('STYLE');

Utils.removeStylesheet(_id)
style = document.querySelector('STYLE[gs-style-id=' + _id + ']');
expect(style).toBe(null);
});

});

describe('test parseHeight', function() {

it('should parse height value', function() {
Expand Down
3 changes: 2 additions & 1 deletion src/dd-resizable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,12 +201,13 @@ export class DDResizable extends DDBaseImplement implements HTMLElementExtendOpt
/** @internal */
protected _resizeStop(event: MouseEvent): DDResizable {
const ev = Utils.initEvent<MouseEvent>(event, { type: 'resizestop', target: this.el });
// Remove style attr now, so the stop handler can rebuild style attrs
this._cleanHelper();
if (this.option.stop) {
this.option.stop(ev); // Note: ui() not used by gridstack so don't pass
}
this.el.classList.remove('ui-resizable-resizing');
this.triggerEvent('resizestop', ev);
this._cleanHelper();
delete this.startEvent;
delete this.originalRect;
delete this.temporalRect;
Expand Down
31 changes: 31 additions & 0 deletions src/gridstack.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ $animation_speed: .3s !default;
.grid-stack > .grid-stack-item {
position: absolute;
padding: 0;
height: var(--gs-cell-height);

> .grid-stack-item-content {
margin: 0;
Expand Down Expand Up @@ -98,6 +99,25 @@ $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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from the old code:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
> .ui-resizable-s
, > .ui-resizable-se
, > .ui-resizable-sw {
bottom: var(--gs-item-margin-bottom);
}
> .ui-resizable-ne,
> .ui-resizable-e,
> .ui-resizable-se {
right: var(--gs-item-margin-right);
}
> .ui-resizable-nw,
> .ui-resizable-w,
> .ui-resizable-sw {
left: var(--gs-item-margin-left);
}

&.ui-draggable-dragging {
&> .ui-resizable-handle {
display: none !important;
Expand Down Expand Up @@ -155,3 +175,14 @@ $animation_speed: .3s !default;
.gs-1 > .grid-stack-item {
width: 100%;
}

.grid-stack {
> .grid-stack-item > .grid-stack-item-content,
> .grid-stack-placeholder > .placeholder-content {
top: var(--gs-item-margin-top);
right: var(--gs-item-margin-right);
bottom: var(--gs-item-margin-bottom);
left: var(--gs-item-margin-left);
}
}

123 changes: 42 additions & 81 deletions src/gridstack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,6 @@ export interface CellPosition {
y: number;
}

interface GridCSSStyleSheet extends CSSStyleSheet {
_max?: number; // internal tracker of the max # of rows we created
}

// extend with internal fields we need - TODO: move other items in here
interface InternalGridStackOptions extends GridStackOptions {
_alwaysShowResizeHandle?: true | false | 'mobile'; // so we can restore for save
Expand Down Expand Up @@ -247,8 +243,6 @@ export class GridStack {
protected _ignoreLayoutsNodeChange: boolean;
/** @internal */
public _gsEventHandler = {};
/** @internal */
protected _styles: GridCSSStyleSheet;
/** @internal flag to keep cells square during resize */
protected _isAutoCellHeight: boolean;
/** @internal limit auto cell resizing method */
Expand Down Expand Up @@ -395,8 +389,6 @@ export class GridStack {
float: opts.float,
maxRow: opts.maxRow,
onChange: (cbNodes) => {
let maxH = 0;
this.engine.nodes.forEach(n => { maxH = Math.max(maxH, n.y + n.h) });
cbNodes.forEach(n => {
const el = n.el;
if (!el) return;
Expand All @@ -407,12 +399,12 @@ export class GridStack {
this._writePosAttr(el, n);
}
});
this._updateStyles(false, maxH); // false = don't recreate, just append if need be
this._updateStyles();
}
});

// create initial global styles BEFORE loading children so resizeToContent margin can be calculated correctly
this._updateStyles(false, 0);
this._updateStyles();

if (opts.auto) {
this.batchUpdate(); // prevent in between re-layout #1535 TODO: this only set float=true, need to prevent collision check...
Expand Down Expand Up @@ -862,7 +854,7 @@ export class GridStack {
this.resizeToContentCheck();

if (update) {
this._updateStyles(true); // true = force re-create for current # of rows
this._updateStyles();
}
return this;
}
Expand Down Expand Up @@ -979,7 +971,6 @@ export class GridStack {
} else {
this.el.parentNode.removeChild(this.el);
}
this._removeStylesheet();
if (this.parentGridNode) delete this.parentGridNode.subGrid;
delete this.parentGridNode;
delete this.opts;
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}
delete w.content;
Expand Down Expand Up @@ -1467,7 +1458,7 @@ export class GridStack {
this.opts.marginTop = this.opts.marginBottom = this.opts.marginLeft = this.opts.marginRight = undefined;
this._initMargin();

this._updateStyles(true); // true = force re-create
this._updateStyles();

return this;
}
Expand Down Expand Up @@ -1547,78 +1538,31 @@ export class GridStack {
return this;
}

/** @internal called to delete the current dynamic style sheet used for our layout */
protected _removeStylesheet(): GridStack {

if (this._styles) {
const styleLocation = this.opts.styleInHead ? undefined : this.el.parentNode as HTMLElement;
Utils.removeStylesheet(this._styleSheetClass, styleLocation);
delete this._styles;
}
return this;
private setVar(el: HTMLElement, varName: string, varValue: string) {
el.style.setProperty(varName, varValue);
}

/** @internal updated/create the CSS styles for row based layout and initial margin setting */
protected _updateStyles(forceUpdate = false, maxH?: number): GridStack {
// call to delete existing one if we change cellHeight / margin
if (forceUpdate) {
this._removeStylesheet();
}

if (maxH === undefined) maxH = this.getRow();
/**
* Updates the CSS variables (used in CSS and inline style) for row based layout and initial margin setting,
* Variables are scoped in DOM so they works for nested grids as well
* @internal
*/
protected _updateStyles(): GridStack {
this._updateContainerHeight();

// if user is telling us they will handle the CSS themselves by setting heights to 0. Do we need this opts really ??
if (this.opts.cellHeight === 0) {
return this;
}

const cellHeight = this.opts.cellHeight as number;
const cellHeightUnit = this.opts.cellHeightUnit;
const prefix = `.${this._styleSheetClass} > .${this.opts.itemClass}`;

// create one as needed
if (!this._styles) {
// insert style to parent (instead of 'head' by default) to support WebComponent
const styleLocation = this.opts.styleInHead ? undefined : this.el.parentNode as HTMLElement;
this._styles = Utils.createStylesheet(this._styleSheetClass, styleLocation, {
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
nonce: this.opts.nonce,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nonce: does that need to be done elsewhere now ?

});
if (!this._styles) return this;
this._styles._max = 0;

// these are done once only
Utils.addCSSRule(this._styles, prefix, `height: ${cellHeight}${cellHeightUnit}`);
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
// content margins
const top: string = this.opts.marginTop + this.opts.marginUnit;
const bottom: string = this.opts.marginBottom + this.opts.marginUnit;
const right: string = this.opts.marginRight + this.opts.marginUnit;
const left: string = this.opts.marginLeft + this.opts.marginUnit;
const content = `${prefix} > .grid-stack-item-content`;
const placeholder = `.${this._styleSheetClass} > .grid-stack-placeholder > .placeholder-content`;
Utils.addCSSRule(this._styles, content, `top: ${top}; right: ${right}; bottom: ${bottom}; left: ${left};`);
Utils.addCSSRule(this._styles, placeholder, `top: ${top}; right: ${right}; bottom: ${bottom}; left: ${left};`);
// resize handles offset (to match margin)
Utils.addCSSRule(this._styles, `${prefix} > .ui-resizable-n`, `top: ${top};`);
Utils.addCSSRule(this._styles, `${prefix} > .ui-resizable-s`, `bottom: ${bottom}`);
Utils.addCSSRule(this._styles, `${prefix} > .ui-resizable-ne`, `right: ${right}`);
Utils.addCSSRule(this._styles, `${prefix} > .ui-resizable-e`, `right: ${right}`);
Utils.addCSSRule(this._styles, `${prefix} > .ui-resizable-se`, `right: ${right}; bottom: ${bottom}`);
Utils.addCSSRule(this._styles, `${prefix} > .ui-resizable-nw`, `left: ${left}`);
Utils.addCSSRule(this._styles, `${prefix} > .ui-resizable-w`, `left: ${left}`);
Utils.addCSSRule(this._styles, `${prefix} > .ui-resizable-sw`, `left: ${left}; bottom: ${bottom}`);
}

// now update the height specific fields
maxH = maxH || this._styles._max;
if (maxH > this._styles._max) {
const getHeight = (rows: number): string => (cellHeight * rows) + cellHeightUnit;
for (let i = this._styles._max + 1; i <= maxH; i++) { // start at 1
Utils.addCSSRule(this._styles, `${prefix}[gs-y="${i}"]`, `top: ${getHeight(i)}`);
Utils.addCSSRule(this._styles, `${prefix}[gs-h="${i + 1}"]`, `height: ${getHeight(i + 1)}`); // start at 2
}
this._styles._max = maxH;
}
// Set CSS var of cell height
this.setVar(this.el, "--gs-cell-height", `${this.opts.cellHeight}${this.opts.cellHeightUnit}`);
// content margins
this.setVar(this.el, "--gs-item-margin-top", `${this.opts.marginTop}${this.opts.marginUnit}`);
this.setVar(this.el, "--gs-item-margin-bottom", `${this.opts.marginBottom}${this.opts.marginUnit}`);
this.setVar(this.el, "--gs-item-margin-right", `${this.opts.marginRight}${this.opts.marginUnit}`);
this.setVar(this.el, "--gs-item-margin-left", `${this.opts.marginLeft}${this.opts.marginUnit}`);

return this;
}

Expand Down Expand Up @@ -1677,17 +1621,32 @@ export class GridStack {
return this;
}

/** @internal call to write position x,y,w,h attributes back to element */
protected _writePosAttr(el: HTMLElement, n: GridStackPosition): GridStack {
/**
* Call to write position x,y,w,h attributes back to element
* In addition, updates the inline top/height inline style as well
* @internal
*/
protected _writePosAttr(el: HTMLElement, n: GridStackNode): GridStack {
if (n.x !== undefined && n.x !== null) { el.setAttribute('gs-x', String(n.x)); }
if (n.y !== undefined && n.y !== null) { el.setAttribute('gs-y', String(n.y)); }
n.w > 1 ? el.setAttribute('gs-w', String(n.w)) : el.removeAttribute('gs-w');
n.h > 1 ? el.setAttribute('gs-h', String(n.h)) : el.removeAttribute('gs-h');
// Avoid overwriting the inline style of the element during drag/resize, but always update the placeholder
if ((!n._moving && !n._resizing) || this._placeholder === el) {
// Set inline style, refer CSS variables
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
el.style.top = `calc(${n.y} * var(--gs-cell-height))`;
if (n.h !== 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

el.style.height = `calc(${n.h} * var(--gs-cell-height))`;
} else {
// height is set to --gs-cell-height by default
delete el.style.height;
}
}
return this;
}

/** @internal call to write any default attributes back to element */
protected _writeAttr(el: HTMLElement, node: GridStackWidget): GridStack {
protected _writeAttr(el: HTMLElement, node: GridStackNode): GridStack {
if (!node) return this;
this._writePosAttr(el, node);

Expand Down Expand Up @@ -2306,7 +2265,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
this._updateContainerHeight();
}
Expand Down Expand Up @@ -2395,6 +2354,7 @@ export class GridStack {
const onEndMoving = (event: Event) => {
this.placeholder.remove();
delete node._moving;
delete node._resizing;
delete node._event;
delete node._lastTried;
const widthChanged = node.w !== node._orig.w;
Expand Down Expand Up @@ -2494,6 +2454,7 @@ export class GridStack {
node._lastUiPosition = ui.position;
node._prevYPix = ui.position.top;
node._moving = (event.type === 'dragstart'); // 'dropover' are not initially moving so they can go exactly where they enter (will push stuff out of the way)
node._resizing = (event.type === 'resizestart');
delete node._lastTried;

if (event.type === 'dropover' && node._temporaryRemoved) {
Expand Down
Loading