hammerlab / cycledash

Variant Caller Analysis Dashboard and Data Management System
Other
35 stars 2 forks source link

Use floatThead for sticky VCF header #800

Closed jaclynperrone closed 8 years ago

jaclynperrone commented 9 years ago

Here's the first attempt at adding the floatThead plugin to make the VCF table header sticky. It's looking pretty good! And the implementation was simple. There are a few known issues though, that I would love some feedback on. It would be great if you can check it out on your local machine, and play around with it/break it:

Things to be done:

Review on Reviewable

jaclynperrone commented 9 years ago

cc: @ihodes

jaclynperrone commented 9 years ago

@danvk As we talked about, I implemented your NPM jQuery patch and it works in the browser. But the tests are failing because jQuery is undefined in jquery.floatThead.js.

ihodes commented 9 years ago

For posterity, performance of this plugin on my MacBook Air: http://link.isaachodes.io/image/1N330J0D3o0L

mkoryak commented 9 years ago

whats the holdup ?

@ihodes the performance can be improved if you set useAbsolutePositioning: false when you init the plugin. The downside to using that mode is if the position of the table moves in the dom, you will have to trigger a reflow event on the table to tell floatThead to update its location.

if you can create a jsfiddle with that table somewhere, i can take a look and see why its being slow.

armish commented 9 years ago

This is what I have on my MacBook Pro (sorry no Air to test this one): screen recording 2015-08-21 at 01 44 pm

It is not as bad as what @ihodes have, but still a little bit laggy. Maybe the bigger tables (the number of variants in the table) make the performance issue much more prominent?

mkoryak commented 9 years ago

you need to init the plugin so that it uses fixed positioning, so that when you scroll, the floated header will have position:fixed;top:0 then It will not do what you are seeing. Absolute positioning works well with overflow scrolling, but that is not what you are doing here.

The latest version of the plugin will pick a positioning mode that best suits the application, or use the option I mentioned earlier.

armish commented 9 years ago

Thanks a lot for the information, @mkoryak! Folks working on this feature are on vacation right now -- hence the lack of updates; but we will give it another try with your suggested optimizations once everybody is back again.

jaclynperrone commented 9 years ago

@armish That's the same kind of jumpiness that I get on my laptop, too. It only happens for me with tables that are super long, with 10,000+ records.

Thanks for the advice, @mkoryak! Making the thead position:fixed causes the columns inside of it to lose their widths and collapse: http://screencast.com/t/MOLYETxdJCuu. Thankfully this plugin accounts for that by calculating and maintaining the widths for you, but the real issue is when the table has overflow-x. Position:fixed takes the thead out of flow and locks it into place, separating it from the horizontally scrolling table. So we went with this solution that avoids position:fixed to account for tables with a ton of columns. If you know of any other work arounds, feel free to send them our way!

Thanks!

mkoryak commented 9 years ago

@jaclynperrone when I recommended using fixed position, I meant configuring the plugin with useAbsolutePositioning: false, not setting that style yourself.

The plugin can run on big tables and perform well, given that it configured properly. see:

http://mkoryak.github.io/floatThead/examples/hidden-columns/

or this page:

http://mkoryak.github.io/floatThead/tests/big-table/

open the inspector and click "float it" and then view the "init" profile on the profiles tab. For me the runtime of $.fn.floatThead is 48ms (2.6ghz macbook pro)

jaclynperrone commented 9 years ago

@mkoryak I tried the useAbsolutePositioning: false configuration but the table fails to load and I get this error: screen shot 2015-09-01 at 3 40 50 pm We only get this error when using this option.

We are using React and are calling the floatThead() function in the componentDidMount method: screen shot 2015-09-01 at 3 44 15 pm

We were wondering if you've used the floatThead plugin with React before, and if so, do you have any tips on getting it up and running?

Thanks!

mkoryak commented 9 years ago

@jaclynperrone no I have not used it with react before. The plugin creates a wrapper div when you init it in that mode, which is probably why react gets upset. There must be a way to use libs that do this in react. I will google a bit. If you can create a jsfiddle that reproduces this issue, I can see if I can fix it there.

how about: http://stackoverflow.com/questions/25436445/using-jquery-plugins-that-transform-the-dom-in-react-components

another approach might be to modify floatThead and add an option where you can specify your own table wrapper div, that way you can create that div in react and floatThead wont need to make one. That might be the best course here.

jaclynperrone commented 9 years ago

@mkoryak Thanks for looking into this! It's weird that we only see these React errors when using the useAbsolutePositioning: false option. Any idea as to why this might be the case? Is it because it needs to be coupled with the reflow event in order to work?

mkoryak commented 9 years ago

@jaclynperrone it works in that mode because the table it not wrapped by container div, most likely this conditional is never entered: https://github.com/mkoryak/floatThead/blob/master/jquery.floatThead.js#L307

try overwriting your local plugin code with this: https://gist.github.com/mkoryak/f24c579ae5f32a5cdd58

and make sure that the table is wrapped in a div that looks like this: <div style="overflow: hidden;" aria-hidden="true" class="floatThead-floatContainer">... your table... </div>

if that works, ill add an option to let you do that

jaclynperrone commented 9 years ago

@mkoryak I took the steps you suggested (thank you again!), but we are seeing errors on line 323 and 205 from the plugin override: floatthead

which is causing an error on line 5214 in jquery.js: jquery

As always, thank you for your time and patience :)

mkoryak commented 9 years ago

ah, that is because I never ran the code. maybe I should do that first :) Let me get back to you

jaclynperrone commented 9 years ago

Hi @mkoryak! I was wondering if you had a chance to look at this? If not, no worries. Just wanted to check-in.

Thanks :)

mkoryak commented 9 years ago

Sorry, busy with work stuff lately. This is still on my radar

ihodes commented 8 years ago

Closing this; we can resurrect this if needed. I'm sorry this didn't work out!