Skip to content

fix(grid): fix mat-grid-tile position bug 11774 #12980

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

Merged
merged 4 commits into from
Sep 21, 2018
Merged

fix(grid): fix mat-grid-tile position bug 11774 #12980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f01d80
fix(grid): fix mat-grid-tile position bug 11774
Sep 5, 2018
57e1168
added unit testing
Sep 5, 2018
54113d4
Fixed one more issue with a single cell tile at the beginning of line
Sep 8, 2018
e1df8b4
add more fix to handle when a single cell should be placed at the
Sep 10, 2018
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
40 changes: 40 additions & 0 deletions src/demo-app/grid-list/grid-list-demo.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,46 @@
</mat-card-content>
</mat-card>

<mat-card>
<mat-card-title>Grid with 1 cell at the beginning of a new row</mat-card-title>
<mat-card-content class="demo-basic-list">
<mat-grid-list [cols]="6" gutterSize="20px" rowHeight="20px">
<mat-grid-tile [colspan]="3" [rowspan]="4"
class="mat-elevation-z15">
</mat-grid-tile>
<mat-grid-tile [colspan]="3" [rowspan]="4"
class="mat-elevation-z15">
</mat-grid-tile>
<mat-grid-tile [colspan]="1" [rowspan]="2"
class="mat-elevation-z15">
</mat-grid-tile>
</mat-grid-list>
</mat-card-content>
</mat-card>

<mat-card>
<mat-card-title>Grid with col-span</mat-card-title>
<mat-card-content class="demo-basic-list">
<mat-grid-list [cols]="10" gutterSize="20px" rowHeight="20px">
<mat-grid-tile [colspan]="4" [rowspan]="4"
class="mat-elevation-z15">
</mat-grid-tile>
<mat-grid-tile [colspan]="2" [rowspan]="2"
class="mat-elevation-z15">
</mat-grid-tile>
<mat-grid-tile [colspan]="4" [rowspan]="4"
class="mat-elevation-z15">
</mat-grid-tile>
<mat-grid-tile [colspan]="4" [rowspan]="4"
class="mat-elevation-z15">
</mat-grid-tile>
<mat-grid-tile [colspan]="4" [rowspan]="4"
class="mat-elevation-z15">
</mat-grid-tile>
</mat-grid-list>
</mat-card-content>
</mat-card>

<mat-card>
<mat-card-title>Fixed-height grid list</mat-card-title>
<mat-card-content>
Expand Down
82 changes: 82 additions & 0 deletions src/lib/grid-list/grid-list.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,65 @@ describe('MatGridList', () => {
expect(getStyle(tiles[3], 'top')).toBe('101px');
});

it('should lay out tiles correctly', () => {
const fixture = createComponent(GridListWithLayout);

fixture.detectChanges();
const tiles = fixture.debugElement.queryAll(By.css('mat-grid-tile'));

expect(getStyle(tiles[0], 'width')).toBe('40px');
expect(getStyle(tiles[0], 'height')).toBe('40px');
expect(getComputedLeft(tiles[0])).toBe(0);
expect(getStyle(tiles[0], 'top')).toBe('0px');

expect(getStyle(tiles[1], 'width')).toBe('20px');
expect(getStyle(tiles[1], 'height')).toBe('20px');
expect(getComputedLeft(tiles[1])).toBe(40);
expect(getStyle(tiles[1], 'top')).toBe('0px');

expect(getStyle(tiles[2], 'width')).toBe('40px');
expect(getStyle(tiles[2], 'height')).toBe('40px');
expect(getComputedLeft(tiles[2])).toBe(60);
expect(getStyle(tiles[2], 'top')).toBe('0px');

expect(getStyle(tiles[3], 'width')).toBe('40px');
expect(getStyle(tiles[3], 'height')).toBe('40px');
expect(getComputedLeft(tiles[3])).toBe(0);
expect(getStyle(tiles[3], 'top')).toBe('40px');

expect(getStyle(tiles[4], 'width')).toBe('40px');
expect(getStyle(tiles[4], 'height')).toBe('40px');
expect(getComputedLeft(tiles[4])).toBe(40);
expect(getStyle(tiles[4], 'top')).toBe('40px');
});

it('should lay out tiles correctly when single cell to be placed at the beginning',
() => {
const fixture = createComponent(GridListWithSingleCellAtBeginning);

fixture.detectChanges();
const tiles = fixture.debugElement.queryAll(By.css('mat-grid-tile'));

expect(getStyle(tiles[0], 'width')).toBe('40px');
expect(getStyle(tiles[0], 'height')).toBe('40px');
expect(getComputedLeft(tiles[0])).toBe(0);
expect(getStyle(tiles[0], 'top')).toBe('0px');

expect(getStyle(tiles[1], 'width')).toBe('20px');
expect(getStyle(tiles[1], 'height')).toBe('40px');
expect(getComputedLeft(tiles[1])).toBe(40);
expect(getStyle(tiles[1], 'top')).toBe('0px');

expect(getStyle(tiles[2], 'width')).toBe('40px');
expect(getStyle(tiles[2], 'height')).toBe('40px');
expect(getComputedLeft(tiles[2])).toBe(60);
expect(getStyle(tiles[2], 'top')).toBe('0px');

expect(getStyle(tiles[3], 'height')).toBe('20px');
expect(getComputedLeft(tiles[3])).toBe(0);
expect(getStyle(tiles[3], 'top')).toBe('40px');
});

it('should add not add any classes to footers without lines', () => {
const fixture = createComponent(GridListWithFootersWithoutLines);
fixture.detectChanges();
Expand Down Expand Up @@ -474,6 +533,29 @@ class GridListWithComplexLayout {
tiles: any[];
}

@Component({template: `
<div style="width:100px">
<mat-grid-list [cols]="10" gutterSize="0px" rowHeight="10px">
<mat-grid-tile [colspan]="4" [rowspan]="4"></mat-grid-tile>
<mat-grid-tile [colspan]="2" [rowspan]="2"></mat-grid-tile>
<mat-grid-tile [colspan]="4" [rowspan]="4"></mat-grid-tile>
<mat-grid-tile [colspan]="4" [rowspan]="4"></mat-grid-tile>
<mat-grid-tile [colspan]="4" [rowspan]="4"></mat-grid-tile>
</mat-grid-list>
</div>`})
class GridListWithLayout {}

@Component({template: `
<div style="width:100px">
<mat-grid-list [cols]="10" gutterSize="0px" rowHeight="10px">
<mat-grid-tile [colspan]="4" [rowspan]="4"></mat-grid-tile>
<mat-grid-tile [colspan]="2" [rowspan]="4"></mat-grid-tile>
<mat-grid-tile [colspan]="4" [rowspan]="4"></mat-grid-tile>
<mat-grid-tile [colspan]="1" [rowspan]="2"></mat-grid-tile>
</mat-grid-list>
</div>`})
class GridListWithSingleCellAtBeginning {}

@Component({template: `
<mat-grid-list cols="1">
<mat-grid-tile>
Expand Down
9 changes: 7 additions & 2 deletions src/lib/grid-list/tile-coordinator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ export class TileCoordinator {
// If we've reached the end of the row, go to the next row.
if (this.columnIndex + tileCols > this.tracker.length) {
this._nextRow();
gapStartIndex = this.tracker.indexOf(0, this.columnIndex);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure whether this makes sense. What it does is that it looks for the value 0, starting from the columnIndex. Since the array is filled with zeroes (see https://github.com/angular/material2/pull/12980/files#diff-ddccaadf0ea62282a345db62650d07f7R58), it'll always stop at the next match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The array is not always filled with 0. In this test case, the row 3 and 4 will only have 0 in the cell 5 and 6. Which is the empty cell in that row.

Copy link
Member

Choose a reason for hiding this comment

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

This line actually isn't going to do anything at all. gapStartIndex is re-written after the continue

gapEndIndex = this._findGapEndIndex(gapStartIndex);
continue;
}

Expand All @@ -99,6 +101,8 @@ export class TileCoordinator {
// If there are no more empty spaces in this row at all, move on to the next row.
if (gapStartIndex == -1) {
this._nextRow();
gapStartIndex = this.tracker.indexOf(0, this.columnIndex);
gapEndIndex = this._findGapEndIndex(gapStartIndex);
Copy link
Member

Choose a reason for hiding this comment

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

I believe this also ins't going to do anything, as these will be rewritten after the continue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

once the code hit continue, it will skip the rest in the block and jump to the "while" check, without the new lines, the endIndex will remain as the last row.

continue;
}

Expand All @@ -108,8 +112,9 @@ export class TileCoordinator {
// gap on the next iteration.
this.columnIndex = gapStartIndex + 1;

// Continue iterating until we find a gap wide enough for this tile.
} while (gapEndIndex - gapStartIndex < tileCols);
// Continue iterating until we find a gap wide enough for this tile. Since gapEndIndex is
// exclusive, gapEndIndex is 0 means we didn't find a gap and should continue.
} while ((gapEndIndex - gapStartIndex < tileCols) || (gapEndIndex == 0));

// If we still didn't manage to find a gap, ensure that the index is
// at least zero so the tile doesn't get pulled out of the grid.
Expand Down