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

Fix bug scroll (up and down) does not propagating to parent #3666

Closed
wants to merge 1 commit into from

Conversation

Jacquelin
Copy link

Hello,

This pull request is to fix the issue #3340.

My first pull request #3531 was not good enough.

Now, I only check if the Vertical scroll length is negative. If it is, then I let the parent the parent container scroll.

Sorry for this double Pull request ...

@PaulL1
Copy link
Contributor

PaulL1 commented Jun 3, 2015

Thanks for the contribution.

My concern is that I'm not sure that getVerticalScrollLength should be returning < 0, so there's a risk that someone will fix that, and thereby break this again.

Also, if we were to merge it we'd need the three commits squashed into one.

@swalters, do you have an opinion?

@Jacquelin
Copy link
Author

PaulL1 said :

My concern is that I'm not sure that getVerticalScrollLength should be returning < 0, so there's a risk that someone will fix that, and thereby break this again.

You are right. So I test some code fix with getVerticalScrollLength as an absolute value. This fix some scroll but not all. I try with getVerticalScrollLength === 0, but same, it's fix some scroll but not all ... Waiting for @swalters response

Also, if we were to merge it we'd need the three commits squashed into one.

For this I don't know how I can do it ...

Thanks for the contribution.

it's a pleasure ;-)

@PaulL1
Copy link
Contributor

PaulL1 commented Jun 4, 2015

@Jacquelin: thanks for the input, we'll wait for @swalters to see what he can tell us.

On the squash, the basic process is to issue git rebase -i upstream/master, this will take you into an interactive process in which you can tell git to squash all your commits into a single commit, and rewrite the commit message that goes with that. You then git push -f to force the remote to take the rewritten commit log.

…ll length are negative (Mean no scroll)

Update of fix scroll on top/bottom : add a check if vertical scroll length is negative, mean 'Let the parent container scroll
@Jacquelin
Copy link
Author

Thank you for the squash !!!!!! ;-)

@swalters
Copy link
Contributor

swalters commented Jun 9, 2015

@Jacquelin I have to agree with @PaulL1 . What should be fixed is the ScrollEvent.AtTop and .AtBottom. Those should both return true if the grid is truly at top and/or bottom row.

@Jacquelin
Copy link
Author

@swalters Oki !

But how can I fix it ?

When rowContainer.getVerticalScrollLength() < 0, So the 'y.percentage' is mixed (or reverse). and this make :

// When on top : this.y.percentage === 1, then atTop return false.
ScrollEvent.prototype.atTop = function(scrollTop) {
  return (this.y && this.y.percentage === 0 && scrollTop === 0);
};
// When on bottom : this.y.percentage === 0 so atBottom return false
ScrollEvent.prototype.atBottom = function(scrollTop) {
  return (this.y && this.y.percentage === 1 && scrollTop > 0);
};

What do you think if I set the scrollYpercentage before call atTop/Bottom ?

// Keep scrollPercentage within the range 0-1.
if (rowContainer.getVerticalScrollLength() < 0) {
  if (scrollTop > 0) { scrollYPercentage = 1; }
  else { scrollYPercentage = 0; }
}
else if (scrollYPercentage < 0) { scrollYPercentage = 0; }
else if (scrollYPercentage > 1) { scrollYPercentage = 1; }

@swalters
Copy link
Contributor

I made a comment in #3690 I think we'll go that direction for the fix. Thanks for the PR

@swalters swalters closed this Jun 18, 2015
swalters added a commit that referenced this pull request Jun 19, 2015
thanks to @500tech-user and @Jacquelin for PR's that led to this fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants