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

Average items height problem #68

Closed
dhilt opened this issue Mar 19, 2016 · 16 comments
Closed

Average items height problem #68

dhilt opened this issue Mar 19, 2016 · 16 comments

Comments

@dhilt
Copy link
Member

dhilt commented Mar 19, 2016

In issue#10 we have a link to some repro -- https://github.com/AlSherif/AngularUIScrollDemo -- and I researched a problem and found out that on a big list with different height items it is possible that the actual error of average items height calculation affects process of scrolling itself. It causes infinite scrollbar jumping in some cases, the repro demonstrates it well.

Padding elements adjustement (Viewport.adjustPadding()) may throw us in a backward direction while we are scrolling forward.

upd: Another one demo -- which is more easy to run -- that shows us the same problem: https://jsfiddle.net/MartyBolton/s38zdu8c/6/. This one cames from issue#65.

@dhilt dhilt changed the title Average items height Average items height problem Mar 19, 2016
@dhilt
Copy link
Member Author

dhilt commented Mar 19, 2016

I tried to avoid average item height recalculation during scrolling and it doesn't work. So I developed another approach. Here the buffer item height is being cached before the item is being removed, then the precise values of padding element heights are being calculated based on cached values of heights of items that were removed from a viewport once.

I pushed some code into paddings-branch that I created for this issue. It does not break tests and it fixes the problem described above... @mfeingold could you please take a look at the branch?

@MartyBolton
Copy link

Hi @dhilt I tested against my repro with random item heights and it looks like your padding-branch is working excellent! Here's the forked repro showing fix.
https://jsfiddle.net/MartyBolton/czf6qvmL/1/

You guys have done some great work on this component. This is the only scroller that I found addresses my needs. I'm thinking it is far beyond other infinite scrollers out there and I looked a a few. Keep up the great work.
Marty

@MartyBolton
Copy link

@dhilt Hey Denis, something else you fixed with padding branch. Hard to explain but in master, FireFox scroll mouse wheel step/speed was very slow, i.e. so it would take forever to scroll one item but in the paddings branch the scroll seems normal again. Just thought I would mention it if you heard that issue before.

@dhilt
Copy link
Member Author

dhilt commented Mar 25, 2016

@MartyBolton I've never heard about such FF issue before. Btw, yes, I see that mouse wheel in FF works a bit slower than in Chrome. But when I'm trying to compare 'master' and 'paddings' scroller functionality on this FF feature I see no difference. How could you see such difference? It would be intrested to look at some repro...

@MartyBolton
Copy link

@dhilt So, hard to explain. I'm building a cordova app and use chrome for the most part and push to android for now, however when I played with how this looks in desktop, I browsed my file based www site with FF because I know FF allows it and found the scrolling was essentially a crawl with master however that was fixed with paddings. The scroll bar looks better now too, although that previously was not an issue it just seemed to be at the bottom more. Now it looks better. I have not tested any of the repros with FF. I'm pretty much targeting cross brower AND devices with Cordova so I'll try to keep you posted on stuff. So far android web view has been same as chrome which is good, scrolling works pretty darn good now. For me the major issue was getting view port height to take up screen so I used a directive called fill-height. That seems to work well but I'm thinking of going CSS route when I get time.

I have to nice to have features if you have any ideas.

a. incorporate a drag-to-refresh option at some point. Today I have a refresh button. But was looking at ui-scroller (a while back) for hardware scrolling - maybe now that paddings is good, I may go back to trying that in conjunction with ui-scroll. Or if you have any ideas. I need to just do a pull-down refresh.

b. for isloading, I'd like to put a spinner in the line element as its loading. Right now I use an ng-show spinner that takes up the entire page. Ideally want to show spinner on where ui-scroll is loading new content. Not sure if you have any plans for a callback or something for that through the adapter so I can put a spinner up on loading only items.
ttyl

@soboko
Copy link

soboko commented Mar 26, 2016

@dhilt I cherry-picked your commit into my fork of ui-scroll and I can confirm that this solved my problem - thanks! The data we're trying to display mostly consists of one-line items (but with occasional 60-80 line items thrown in) and I was getting infinite scrollbar jumping when I hit those large items when scrolling down (not scrolling up, though).

@mrova
Copy link

mrova commented Mar 29, 2016

Works for me too! But...looks like it broke #70 :(

@dhilt
Copy link
Member Author

dhilt commented Mar 30, 2016

@MartyBolton thanks a lot for the explanation and your ideas! we have to think about it.

@mrova what exactly is broken here, could you give us some details on this?

@mrova
Copy link

mrova commented Mar 30, 2016

@dhilt so...
on v1.3.3 when I scroll fast back and forth empty space appears on top/bottom like on this example http://screencast.com/t/O0YkrmbDKvW
On master branch this empty space is not shown.
When merged paddings branch to master it's broken again (the empty space) but flickering is gone.

@studentIvan
Copy link

@mrova do you use adapter append? I had same problem in this case and temporary solve it by commented clipBottom method

@dhilt
Copy link
Member Author

dhilt commented Apr 1, 2016

@mrova please try the latest code form 'paddings' branch: https://github.com/angular-ui/ui-scroll/tree/paddings/dist -- does it solve your empty space issue? if no, it would be very helpful if you give us a little repro on jsfiddle for example.

We are going to release a new version of ui.scroll right after 'paddings' updates finalizing.

@dhilt dhilt mentioned this issue Apr 1, 2016
@mrova
Copy link

mrova commented Apr 4, 2016

@dhilt I have it on my plate for today.

@dhilt
Copy link
Member Author

dhilt commented Apr 4, 2016

Btw 'paddings' has been merged into 'master'.
Soon it will be released as v1.4.1.

@MartyBolton
Copy link

@dhilt Awesome Denis, you guys rock. Thanks a ton! for working on this issue!

@mrova
Copy link

mrova commented Apr 5, 2016

@dhilt still the same. No flickering but gap is there when I scroll fast.
I'll investigate it on our site and prepare (not)working example on jsfiddle but it will be end of the week more likely.

@dhilt dhilt closed this as completed Apr 17, 2016
@dhilt
Copy link
Member Author

dhilt commented Apr 17, 2016

Fixed in v1.4.1.

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

No branches or pull requests

5 participants