huboard / huboard-web

GitHub issues made awesome
https://huboard.com
61 stars 26 forks source link

Scrollable Columns Performance #360

Closed discorick closed 8 years ago

discorick commented 8 years ago

Adds a ScrollableColumnMixin which fires events for scrolling based on a simple tolerance. Initial POC implementation reveals additional cards during scroll and hides them again on scroll up.

Greatly improves initial performance on Firefox.

TODO:

Known bugs: - Scrolling really quickly can throw the list out of sync - Scrolling upwards while dragging issues breaks if that issue is removed from the array

discorick commented 8 years ago

@rauhryan here is the scrollbar bug I am running into:

scrollbarbug

rauhryan commented 8 years ago

Yeah that's not a bug, that's how OSX does scrollbars :)

discorick commented 8 years ago

Alright things are looking pretty tight. The one bug I have identified is attempting to scroll and item straight to the bottom of the list doesn't work if the list is scrolled - this is a known jquery bug and is fixed in version 1.12.0, we are currently using 1.9.1 for the sortable library.

@rauhryan is it safe to replace our jquery build ? I would just swap it with the newest but I doubt we need all the addons, do you know what we need?

rauhryan commented 8 years ago

@discorick it should be safe to upgrade us to the latest jquery-ui but I'm almost positive we should be on 1.11.4, I swear I upgraded it when I was working on superSortable.... but maybe not

I can give it a shot real quick

discorick commented 8 years ago

@rauhryan That would be awesome, yeah we just need 1.12.x to get the scroll fixes in there.

rauhryan commented 8 years ago

@discorick done

discorick commented 8 years ago

@rauhryan thanks! Unfortunately the upgrade didn't fix as promise :(

Scrolling a card from the top down to the bottom still dissallows dragging beneath the lowest card.

rauhryan commented 8 years ago

It doesn't seem consistent, are sure it's jquery

Sent from my iPhone

On Aug 26, 2016, at 3:18 PM, discorick notifications@github.com wrote:

@rauhryan thanks! Unfortunately the upgrade didn't fix as promise :(

Scrolling a card from the top down to the bottom still dissallows dragging beneath the lowest card.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

discorick commented 8 years ago

Not sure no, just seemed the most likely explanation. Hmm this is another tough one.

dahlbyk commented 8 years ago

short curcuit scrolling if filters are active

Why do we need to do this?

discorick commented 8 years ago

short curcuit scrolling if filters are active

Why do we need to do this?

For the scrolling optimization, we maintain a seperate array that keeps popping in new items (which in turn render to the DOM) as the list scrolls downward.

This is troublesome with filters because we are not able to determine if an item is filtered or not currently unless it is in the DOM - in other words the filter set will only filter on items already on the DOM, and doesn't know whether the next batch of DOM elements should be filtered or not .

rauhryan commented 8 years ago

short curcuit scrolling if filters are active Why do we need to do this?

Because the scrolling handlers push and remove items from visibleIssues with is a computed array

In theory, computed properties should be immutable, but I can see in this case where it gets weird figuring out what properties to observe and compute based off

An alternated would be to recompute the visibleIssues array based on the cardIndex property, but then the list refresh calls have to become some kind of observer based on who knows what and it becomes hard to follow

TL;DR; two-way binding is hard... rendering things is hard...

discorick commented 8 years ago

An alternated would be to recompute the visibleIssues array based on the cardIndex property, but then the list refresh calls have to become some kind of observer based on who knows what and it becomes hard to follow

Even in the case of filters, cards would need to be added to the DOM before we could tell if they matched the current filters