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

Jump to bottom
Merged
merged 4 commits into from
Sep 21, 2018
Merged

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

merged 4 commits into from
Sep 21, 2018

Conversation

vivian-hu-zz
Copy link
Contributor

refresh gapStartIndex and gapEndIndex after move to a new row. Otherwise, they become out of sync and causes wrong calculation.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 5, 2018
@vivian-hu-zz vivian-hu-zz added the action: merge The PR is ready for merge by the caretaker label Sep 5, 2018
@ngbot
Copy link

ngbot bot commented Sep 5, 2018

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure missing required label: "target: *"
    pending status "continuous-integration/travis-ci/pr" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@vivian-hu-zz vivian-hu-zz added the target: patch This PR is targeted for the next patch release label Sep 5, 2018
crisbeto
crisbeto reviewed Sep 5, 2018
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

Can we have unit tests for this?

src/lib/grid-list/tile-coordinator.ts
@@ -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

jelbourn
jelbourn reviewed Sep 5, 2018
View reviewed changes
src/lib/grid-list/tile-coordinator.ts
@@ -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.

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

src/lib/grid-list/tile-coordinator.ts
@@ -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.

vivian-hu-zz
vivian-hu-zz commented Sep 5, 2018
View reviewed changes
Copy link
Contributor Author

@vivian-hu-zz vivian-hu-zz left a comment

Choose a reason for hiding this comment

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

Added unit testing

jelbourn
jelbourn approved these changes Sep 6, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed labels Sep 6, 2018
vivian-hu-zz
vivian-hu-zz commented Sep 10, 2018
View reviewed changes
Copy link
Contributor Author

@vivian-hu-zz vivian-hu-zz left a comment

Choose a reason for hiding this comment

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

handle a single cell at the beginning of a new row correctly.

@mmalerba mmalerba merged commit 966cf5f into angular:master Sep 21, 2018
@vivian-hu-zz vivian-hu-zz mentioned this pull request Oct 16, 2018
Closed
roboshoes pushed a commit to roboshoes/material2 that referenced this pull request Oct 23, 2018
@vivian-hu-zz @roboshoes
fix(grid): fix mat-grid-tile position (angular#12980)
6e98d16 
refresh gapStartIndex and gapEndIndex after move to a new row. Otherwise, they become out of sync and causes wrong calculation.
@vivian-hu-zz vivian-hu-zz deleted the bug11774 branch January 23, 2019 23:01
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto left review comments

@jelbourn jelbourn jelbourn approved these changes

@amcdnl amcdnl Awaiting requested review from amcdnl

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vivian-hu-zz @jelbourn @crisbeto @googlebot @mmalerba