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

Prevent scrollEvent when scroll percentage is greater then 1. #6241

Merged
merged 4 commits into from
Jun 18, 2017

Conversation

rdkleine
Copy link
Contributor

@rdkleine rdkleine commented Jun 1, 2017

In the scenario where cell edit and cell navigation is enabled the scrollPercentage will be greater then 1. But this only happens in the bottom row. The check prevents scrolling when this happens.

Copy link
Member

@mportuga mportuga left a comment

Choose a reason for hiding this comment

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

Could please add unit tests that cover this scenario? I want to make sure that this does not get reintroduced later.

@rdkleine
Copy link
Contributor Author

rdkleine commented Jun 7, 2017

Will add some unittests.
I'm having trouble running unit tests. The task karma:angular-1.4.3 failed with Error "TypeError: Cannot read property 'prototype' of undefined"
#6214 Seems related

@mportuga
Copy link
Member

Weird, the karma tests are passing for me. I will take a look when I have a chance.

@rdkleine
Copy link
Contributor Author

Updated my fork. Tests are running again.

@rdkleine
Copy link
Contributor Author

Added coverage for the changes in grid.js.

@mportuga mportuga merged commit 43ffebb into angular-ui:master Jun 18, 2017
@rdkleine
Copy link
Contributor Author

Cool thanks!

vishalnarewade pushed a commit to vishalnarewade/ui-grid that referenced this pull request Nov 6, 2017
…er then 1. (angular-ui#6241)

* Prevent scrollEvent when scroll percentage is greater then 1.

* Unit tests

* Small adjustment to prevscrolltop
@RobPierce
Copy link
Contributor

This seems to have broken scrollTo - debugging through, when I add a new row to the bottom of my grid, which is off the screen, the scrollToIfNecessary calculates the percentage to scroll to be 1.09 in this particular case and due to this change it means it won't scroll as:

if (percentage <= 1)

is not true. I'm not sure why this change was made but it doesn't seem that the logic is correct - if a row is off the screen then we want to scroll to it; either the logic to determine the percentage value is incorrect or the logic that decides not to scroll if percentage > 1 is wrong.

I'll open a new issue for this and link it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants