Magic-Grid icon indicating copy to clipboard operation
Magic-Grid copied to clipboard

Error in calculating number of columns

Open surimohnot opened this issue 6 years ago • 6 comments

There is a minor but substantial error in calculating number of columns. I will try to explain it below, As per your code, column width = item width + gutter number of columns = parent width / column width However, last column don't need gutter. So it should be number of columns = ( parent width + gutter ) / column width

you may need to change other related code as well.

surimohnot avatar Jul 01 '19 14:07 surimohnot

That's actually not an issue. The gutter is on the left of each column and the first column doesn't get a gutter.

positionItems () {
    let { cols, wSpace } = this.setup();
    let maxHeight = 0;
    let colWidth = this.colWidth();

    wSpace = Math.floor(wSpace / 2);

    for (let i = 0; i < this.items.length; i++) {
      let col = this.nextCol(cols, i);
      let item = this.items[i];
      let topGutter = col.height ? this.gutter : 0;

      // Every column is moved from the left by the value of its index
      // multiplied by the width of a single column (colWidth). The trick 
      // is that the first column has an index of 0 and is not affected by
      // colWidth (0 * colWidth = 0). As a result it has no gutter. If you
      // follow the loop on paper, every other column gets a gutter to the left.
      let left = col.index * colWidth + wSpace + "px";

Is this part of a bigger concern though? Did you get any visual errors?

e-oj avatar Jul 01 '19 15:07 e-oj

I forgot to mention that the "extra gutter" is added to the whitespace (wSpace), which is then split equally between both sides.

setup () {
    let width = this.container.getBoundingClientRect().width;
    let colWidth = this.colWidth();
    let numCols = Math.floor(width/colWidth) || 1;
    let cols = [];

    if (this.maxColumns && numCols > this.maxColumns) {
      numCols = this.maxColumns;
    }

    for (let i = 0; i < numCols; i++) {
      cols[i] = {height: 0, index: i};
    }
    
    // Extra gutter gets added to the overall whitespace
    // which is later split in half and used to center the grid.
    let wSpace = width - numCols * colWidth + this.gutter;

    return {cols, wSpace};
  }

e-oj avatar Jul 01 '19 15:07 e-oj

Yes, this is actually part of a bigger concern, which makes this awesome script practically useless for me. Actually I want to create a grid where columns are having flexible width. So my calculation was as follows,

let's say Gutter = 30px;

Now in my css I have

.column { width: 100%; } @media only screen and (min-width: 640px) { .column { width: calc(50% - 15px); // 30px Gutter for two column layout. } } @media only screen and (min-width: 1080px) { .column { width: calc(33.3333% - 20px); // 60px total gutter shared by three columns. } }

However, your script showing only 1 column above 640px and only 2 columns above 1080px (which should not happen).

Hope I am able to explain correctly.

surimohnot avatar Jul 01 '19 15:07 surimohnot

I'm also questioning the need for this extra gutter, which is then split into half. This results into my grid having always a gap of at least half the gutter on the left and right hand-side, no matter how ideally I size my columns.

Let's take the following scenario: container width=1230px maxColumns=4 gutter=10 column maxWidth=400px

I would expect a perfect alignment from x=0 to x=1230. What I have instead is a gap of 5 at the left of the first column.

filipjnc avatar Jul 11 '19 20:07 filipjnc

I believe I have a fix for this issue, and I opened a PR. See my proposed implementation of a responsive layout test/grid-responsive.html

stevenamoody avatar Jun 17 '20 01:06 stevenamoody

had a similar issue, where my elements wouldn't align properly, even though the math was correct (2 columns, 10px gutter; element width: calc(50% - 5px)). the elements where center stacked (even though center is set to false). stevenamoody's edits solved my issue.

jnz31 avatar Jul 15 '21 15:07 jnz31