Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(virtual-repeat-container): support horizontal scrollbar in vertical orientation #11462

Merged
merged 1 commit into from Mar 6, 2019

Conversation

ghost
Copy link

@ghost ghost commented Sep 26, 2018

Issue #11461 : md-virtual-repeat-container: Scrolling to lasts elements do not render elements

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #11461

What is the new behavior?

Correct redering of last elements.
CodePen Demo added in issue for testing

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Sep 26, 2018
@ghost
Copy link
Author

ghost commented Sep 26, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Sep 26, 2018
@codymikol
Copy link
Contributor

I think you might have linked the wrong issue

@ghost
Copy link
Author

ghost commented Sep 26, 2018

Thats right, missing the last digit of the number. Corrected ! Thanks

@codymikol
Copy link
Contributor

Looks like the CI failed because of some virtual-repeat tests. I would run the tests locally and make sure you're not breaking any existing functionality and adjust your code / the tests appropriately.

@Splaktar
Copy link
Contributor

Splaktar commented Sep 29, 2018

You checked the box for "The commit message follows our guidelines" but you didn't follow the guidelines. Please update your commit message and fix any broken unit tests locally.

@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: unit tests This PR needs unit tests to cover the changes being proposed labels Sep 29, 2018
@Splaktar
Copy link
Contributor

These tests are failing:

Chrome 69.0.3497 (Windows 10.0.0) <md-virtual-repeat> should update topIndex when scrolling FAILED
	Expected -10 to be 0.
	    at UserContext.<anonymous> (src/components/virtualRepeat/virtual-repeater.spec.js:582:28)
Chrome 69.0.3497 (Windows 10.0.0) <md-virtual-repeat> should start at the given topIndex position FAILED
	Expected 0 to be 100.
	    at UserContext.<anonymous> (src/components/virtualRepeat/virtual-repeater.spec.js:604:35)

We'll need all existing tests to pass before we can consider the PR.

@Splaktar Splaktar added the needs: more info The issue does not contain enough information for the team to determine if it is a real bug label Sep 29, 2018
@Splaktar
Copy link
Contributor

It would also be helpful if you could explain your approach and add more details for why it handles all of the use cases better than the previous code.

@Splaktar Splaktar changed the title Update virtual-repeater.js WIP: fix(virtual-repeat-container): programmatic scroll to end causes rendering failures when horizontal scrollbar is applied Sep 29, 2018
@ghost
Copy link
Author

ghost commented Oct 1, 2018

You checked the box for "The commit message follows our guidelines" but you didn't follow the guidelines. Please update your commit message and fix any broken unit tests locally.

Done.

@Splaktar
Copy link
Contributor

Splaktar commented Oct 1, 2018

@JSitjaNCR I see that you made a commit to the master branch in your fork that added the full PR template markdown to the commit message.

First thing is that when you update a commit or make changes to a PR, you'll need to use your same branch (patch-1 in this case). Then when you commit, you will want to amend the previous commit. Then you need to force push your updates to that same branch.

Second thing, your commit message should look like this:

fix(virtual-repeat-container): support horizontal scrollbar in vertical orientation

programmatic scroll to end causes rendering failures
when horizontal scrollbar is applied

Fixes #11461.

As described in https://github.com/angular/material/blob/master/.github/CONTRIBUTING.md#-commit-message-format. It doesn't need to include the whole PR template.

Third, the tests are still failing. Please run npm run test:full locally and resolve any test failures, amend your commit, then force push your updates.

@ghost
Copy link
Author

ghost commented Oct 2, 2018

First thing is that when you update a commit or make changes to a PR, you'll need to use your same branch (patch-1 in this case). Then when you commit, you will want to amend the previous commit. Then you need to force push your updates to that same branch.

Ok, I'm new on Github. I make my first commit directly into the browser without doing locally a clone, and i don't found the branch to amend the previous commit. (Sorry for my bad english)

Third, the tests are still failing. Please run npm run test:full locally and resolve any test failures, amend your commit, then force push your updates.

Done !

@codymikol
Copy link
Contributor

Make sure to push any changes you made to fix tests 😉

@ghost
Copy link
Author

ghost commented Oct 2, 2018

Make sure to push any changes you made to fix tests 😉

Finally found how to clone the branch.
Pushed !
Expect doing the correct thing. 😰

@Splaktar
Copy link
Contributor

Splaktar commented Oct 3, 2018

No problem with being new to GitHub. We're happy to help you work through it or point you to resources.

I'm glad that you were able to get your fork cloned and push and update to your branch! That's good progress. Once you get the hang of these things, it gets a lot easier and faster 😄

Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Please squash your commits as described at the end of the Submitting Pull Requests section of our Pull Request Guidelines.

You can avoid needing to squash commits in the first place by using Git's amend commit feature. This GitHub Guide on Amending Commits is really helpful.

@Splaktar Splaktar self-assigned this Oct 3, 2018
@Splaktar Splaktar added needs: squash commits and removed needs: more info The issue does not contain enough information for the team to determine if it is a real bug labels Oct 3, 2018
…al orientation

programmatic scroll to end causes rendering failures when horizontal scrollbar is applied
amended: passing tests

Fixes #11461.
@Splaktar Splaktar added P5: nice to have These issues will not be fixed without community contributions. and removed needs: squash commits labels Dec 9, 2018
@Splaktar
Copy link
Contributor

Splaktar commented Dec 9, 2018

Restarted the one test run that got disconnected. It looks like they will all pass now. Thank you for fixing the tests.

Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM and manual testing was successful.

@Splaktar Splaktar assigned jelbourn and mmalerba and unassigned jelbourn Dec 20, 2018
@Splaktar Splaktar modified the milestones: 1.1.12, 1.1.13 Jan 3, 2019
@Splaktar Splaktar assigned mmalerba and unassigned andrewseguin Jan 29, 2019
@Splaktar Splaktar modified the milestones: 1.1.13, 1.1.14 Feb 13, 2019
@Splaktar Splaktar assigned andrewseguin and unassigned jelbourn Feb 26, 2019
@josephperrott josephperrott merged commit 3cf4d74 into angular:master Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P4: minor Minor issues. May not be fixed without community contributions. pr: merge ready This PR is ready for a caretaker to review type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants