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 3 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
33 changes: 8 additions & 25 deletions spec/gridstack-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

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

remove those test (no need to test removed code)

Copy link
Member

Choose a reason for hiding this comment

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

just remove - obsolete code.

const 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
GridStack.init(options);
expect(document.querySelector("style")).toBe(null);
});
it('should add STYLE to HEAD if styleInHead === true', function() {
var options = {
it('should not add STYLE to HEAD if styleInHead === true', function() {
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
const 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');
GridStack.init(options);
expect(document.querySelector("style")).toBe(null);
});
});

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
45 changes: 45 additions & 0 deletions src/gridstack.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,3 +155,48 @@ $animation_speed: .3s !default;
.gs-1 > .grid-stack-item {
width: 100%;
}

.grid-stack > .grid-stack-item {
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
height: var(--gs-cell-height);
}

.grid-stack > .grid-stack-item > .grid-stack-item-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);
}

.grid-stack > .grid-stack-placeholder > .placeholder-content {
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
top: var(--gs-item-margin-top);
right: var(--gs-item-margin-right);
bottom: var(--gs-item-margin-bottom);
left: var(--gs-item-margin-left);
}

.grid-stack > .grid-stack-item > .ui-resizable-n {
top: var(--gs-item-margin-top);
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
}
.grid-stack > .grid-stack-item > .ui-resizable-s {
bottom: var(--gs-item-margin-bottom);
}
.grid-stack > .grid-stack-item > .ui-resizable-ne {
right: var(--gs-item-margin-right);
}
.grid-stack > .grid-stack-item > .ui-resizable-e {
right: var(--gs-item-margin-right);
}
.grid-stack > .grid-stack-item > .ui-resizable-se {
right: var(--gs-item-margin-right);
bottom: var(--gs-item-margin-bottom);
}
.grid-stack > .grid-stack-item > .ui-resizable-nw {
left: var(--gs-item-margin-left);
}
.grid-stack > .grid-stack-item > .ui-resizable-w {
left: var(--gs-item-margin-left);
}
.grid-stack > .grid-stack-item > .ui-resizable-sw {
left: var(--gs-item-margin-left);
bottom: var(--gs-item-margin-bottom);
}
126 changes: 40 additions & 86 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.parentElement, "--gs-cell-height", `${this.opts.cellHeight}${this.opts.cellHeightUnit}`);
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
// content margins
this.setVar(this.el.parentElement, "--gs-item-margin-top", `${this.opts.marginTop}${this.opts.marginUnit}`);
this.setVar(this.el.parentElement, "--gs-item-margin-bottom", `${this.opts.marginBottom}${this.opts.marginUnit}`);
this.setVar(this.el.parentElement, "--gs-item-margin-right", `${this.opts.marginRight}${this.opts.marginUnit}`);
this.setVar(this.el.parentElement, "--gs-item-margin-left", `${this.opts.marginLeft}${this.opts.marginUnit}`);

return this;
}

Expand Down Expand Up @@ -1677,17 +1621,27 @@ 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 {
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');
/**
* 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, node: GridStackNode): GridStack {
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
if (node.x !== undefined && node.x !== null) { el.setAttribute('gs-x', String(node.x)); }
if (node.y !== undefined && node.y !== null) { el.setAttribute('gs-y', String(node.y)); }
node.w > 1 ? el.setAttribute('gs-w', String(node.w)) : el.removeAttribute('gs-w');
node.h > 1 ? el.setAttribute('gs-h', String(node.h)) : el.removeAttribute('gs-h');
// Avoid overwriting the inline style of the draggable element, but update the placeholder
if (!node._moving || this._placeholder === el) {
// Set inline style, refer CSS variables
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
el.style.top = `calc(${node.y} * var(--gs-cell-height))`;
el.style.height = `calc(${node.h} * var(--gs-cell-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 +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
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 @@ -2493,7 +2447,7 @@ export class GridStack {
node.el = this.placeholder;
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._moving = (event.type === 'dragstart' || event.type === 'resizestart'); // 'dropover' are not initially moving so they can go exactly where they enter (will push stuff out of the way)
lmartorella marked this conversation as resolved.
Show resolved Hide resolved
delete node._lastTried;

if (event.type === 'dropover' && node._temporaryRemoved) {
Expand Down
5 changes: 3 additions & 2 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ export const gridDefaults: GridStackOptions = {
// handleClass: null,
// removable: false,
// staticGrid: false,
// styleInHead: false,
//removable
};

Expand Down Expand Up @@ -263,7 +262,9 @@ export interface GridStackOptions {
*/
staticGrid?: boolean;

/** if `true` will add style element to `<head>` otherwise will add it to element's parent node (default `false`). */
/**
* @deprecated Not used anymore, styles are now implemented with local CSS variables
*/
styleInHead?: boolean;

/** list of differences in options for automatically created sub-grids under us (inside our grid-items) */
Expand Down