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

Default columns arg for non-12 columns scenarios #2851

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

lmartorella
Copy link
Contributor

Description

Some applications can use default column number (for the largest resolutions) higher than 12.
In that case, during column count dynamic switch due to responsive-based resizes, it can happen that the nodeBoundFix function would try to cap the coordinates to 12 rather than the default max column number.
With this fix it is then possible to subclass GridStackEngine and change the default "max" column, rather having the constants hard-coded in the codebase.

Thx, L.

Checklist

  • Created tests which fail without the change (if possible)
  • All NEW tests passing (yarn test) (unfortunately the main trunk is not stable at the moment)
  • Extended the README / documentation, if necessary

@@ -31,6 +31,7 @@ export class GridStackEngine {
public addedNodes: GridStackNode[] = [];
public removedNodes: GridStackNode[] = [];
public batchMode: boolean;
public defaultColumn = 12;
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of having a 'hard coded' max layout of 12, we can just use the largest size in the layout as the current max. will have to check code again...

@adumesny adumesny merged commit 715b376 into gridstack:master Nov 16, 2024
@adumesny
Copy link
Member

adumesny commented Nov 16, 2024

| With this fix it is then possible to subclass GridStackEngine and change the default "max" column, rather having the constants hard-coded in the codebase.

we shouldn't need to subclass to set the 'max column' layout saving. will add an option of this instead...or infer it from max layout size info.

expect(findNode(engine, "left")).toEqual(jasmine.objectContaining({x: 0, y: 0, w: 1, h: 1}));
expect(findNode(engine, "right")).toEqual(jasmine.objectContaining({x: 0, y: 1, w: 1, h: 1}));
// Resize back to 24 column
engine.column = 24;
Copy link
Member

Choose a reason for hiding this comment

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

note that this would work BEFORE your changes too as you are calling engine.columnChanged() directly with 24 and defaultColumn is not updated to match

@adumesny
Copy link
Member

adumesny commented Nov 17, 2024

@lmartorella this works so not sure we needed this code after all... we save the 24 layout info and reuse it anyway... maybe some corner case adding a single larger item with scaling would be affected, not sure...

this code handled large layouts already...

load() {
....
    if (maxColumn > column) {
      this._ignoreLayoutsNodeChange = true; // skip layout update
      this.engine.cacheLayout(items, maxColumn, true);
    }
}

test that run ok

    it('24 layout in 12 column to 1 back 12 then 24', function() {
      const children: GridStackWidget[] = [{ x: 0, y: 0, w: 12, h: 1, id: 'left' }, { x: 12, y: 0, w: 12, h: 1, id: 'right' }];
      children.forEach((c, i) => c.content = c.id);

      grid = GridStack.init({children});
      const left = find('left');
      const right = find('right');

      // side-by-side components 12+12 = 24 columns but only have 12!
      expect(left).toEqual(jasmine.objectContaining({x: 0, y: 0, w: 12}));
      expect(right).toEqual(jasmine.objectContaining({x: 0, y: 1, w: 12}));
      // Resize to 1 column
      grid.column(1);
      expect(left).toEqual(jasmine.objectContaining({x: 0, y: 0, w: 1}));
      expect(right).toEqual(jasmine.objectContaining({x: 0, y: 1, w: 1}));
      // Resize back to 12 column
      grid.column(12);
      expect(left).toEqual(jasmine.objectContaining({x: 0, y: 0, w: 12}));
      expect(right).toEqual(jasmine.objectContaining({x: 0, y: 1, w: 12}));
      // Resize to 24 column
      grid.column(24);
      expect(left).toEqual(jasmine.objectContaining({x: 0, y: 0, w: 12}));
      expect(right).toEqual(jasmine.objectContaining({x: 12, y: 0, w: 12}));
    });

adumesny added a commit to adumesny/gridstack.js that referenced this pull request Nov 17, 2024
* for gridstack#2851 make sure to update the value and added some better testing (grid, not engine that behaves differently).
@adumesny adumesny mentioned this pull request Nov 17, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants