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

Refresh method not working as expected #85

Closed
adamhammouda opened this Issue Oct 6, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@adamhammouda

adamhammouda commented Oct 6, 2016

I'd suggest changing:

self.getRowsHeight(rows) && self.update(rows);

To:

self.getRowsHeight(rows);
self.update(rows);

To account for cases where row height hasn't changed but scroll position has (e.g. re-positioned manually by setting el.scrollTop).

Or at least an override argument could be passed in to disable the default short-circuit behavior.

@NeXTs

This comment has been minimized.

Show comment
Hide comment
@NeXTs

NeXTs Oct 6, 2016

Owner

Hey there

I don't get your idea
If row height hasn't been changed then there are no reason to call .reshesh.
If you need to set el.scrollTop manually, just do it, you don't need call .refresh in this case.

Owner

NeXTs commented Oct 6, 2016

Hey there

I don't get your idea
If row height hasn't been changed then there are no reason to call .reshesh.
If you need to set el.scrollTop manually, just do it, you don't need call .refresh in this case.

@adamhammouda

This comment has been minimized.

Show comment
Hide comment
@adamhammouda

adamhammouda Oct 6, 2016

At the end of the day it turns out to be more of a browser compatibility issue than anything. Again - I don't think it unreasonable to include [at minimum] an argument to bypass the short-circuit behavior in the refresh function, therefore allowing a user to actually force a full refresh - regardless of internal state - when desired.

Here's an example which demonstrates the issue in the latest Firefox release (tested on Windows 7):

http://imgur.com/a/UBEpn

https://jsfiddle.net/dp0h8hcn/1/

adamhammouda commented Oct 6, 2016

At the end of the day it turns out to be more of a browser compatibility issue than anything. Again - I don't think it unreasonable to include [at minimum] an argument to bypass the short-circuit behavior in the refresh function, therefore allowing a user to actually force a full refresh - regardless of internal state - when desired.

Here's an example which demonstrates the issue in the latest Firefox release (tested on Windows 7):

http://imgur.com/a/UBEpn

https://jsfiddle.net/dp0h8hcn/1/

@NeXTs NeXTs closed this in d6b8206 Oct 7, 2016

@NeXTs

This comment has been minimized.

Show comment
Hide comment
@NeXTs

NeXTs Oct 7, 2016

Owner

Here you are @adamhammouda

Calling .refresh(true) should do the trick

Owner

NeXTs commented Oct 7, 2016

Here you are @adamhammouda

Calling .refresh(true) should do the trick

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