mkoryak / floatThead

Fixed <thead>. Doesn't need any custom css/html. Does what position:sticky can't
https://mkoryak.github.io/floatThead/
MIT License
1.22k stars 198 forks source link

Body shrinking #276

Closed kangax closed 8 years ago

kangax commented 8 years ago

I'm seeing table's body shrink as soon as I apply floatThead. Here's the original state:

screenshot 2015-12-28 12 54 04

and here's what happens after $('table').floatThead():

screenshot 2015-12-28 12 54 18

I'm testing this on ES6 compat table. I've included the lib temporarily so you can see the outcome by running $('table').floatThead() in a console.

Any idea what's going on?

mkoryak commented 8 years ago

Hey! I opened an issue a very long time ago trying to apply my plugin to your table. The problem with your table is this invalid markup:

<colgroup>
        <col>
        <col class="this-browser-col">
</colgroup>

the number of cols within a colgroup should reflect the number of columns in the table. my plugin uses colgroups internally to make the floated header's column widths match those of the table. It does support using an existing colgroup if it exists, this is what it is doing here.

I am not sure why you have that colgroup there, but assuming its there for a good reason, you will need to update it have the same number of col elements as the table.

I remember that I had other issues where some kind of tooltip was floated over the TH of the hovered column, this doesnt appear to be happening anymore, so maybe you just need to fix this one thing

mkoryak commented 8 years ago

I missed your comment about having the lib in there. ill play around with it.

mkoryak commented 8 years ago

ok, i played around with it some more. There are few more issues:

  1. Rules like this wont fly:
#show-obsolete:checked ~ table .obsolete, #show-unstable:checked ~ table .unstable {
    display: table-cell;
}

when the headers are floated, now you have 2 tables in there. The floated headers no longer live in a table next to your checkbox. I like the trick of not using any javascript here, but this will longer work. You need to add js to set a class on the body - and I say the body because the plugin copies the table's classes to the new table which contains the headers so css like this will continue to work:

body.show-unstable .the-giant-table  .unstable {
   display: table-cell;
}

this css will apply to both tables.

  1. The table's td.category cells contain an incorrect number for their colspan when some cells are hidden. The browser is forgiving of colspans greater than the number of columns in the table, but my plugin cant figure out the true number of columns. A quick fix to this problem looks like this:
var cols = $("tr.supertest:first>td:visible").length;
$("tr.category>td").attr('colspan', cols);
  1. I think that the colgroup is probably a relic because removing it doesnt seem to break anything $('colgroup').remove()
kangax commented 8 years ago

Thanks for looking into this! I removed colgroup; will try to fix other issues now too.

kangax commented 8 years ago

Hey, so I pushed few changes under a "#float" flag — http://kangax.github.io/compat-table/es6/#float

This almost works now! Check it out.

One artifact I'm still seeing is this:

screenshot 2015-12-29 12 02 31

I'll look into it next.

kangax commented 8 years ago

Firefox (nightly) seems to have a slight shift:

screenshot 2015-12-29 12 11 09

mkoryak commented 8 years ago

I think the headers not lining up issue has to do with some styles still not applying to the floated header and applying to the main table. The plugin expects that both tables are styled identically.

also, I suggest making this change:

table.floatThead = (function(floatThead) {
    var created = false;
    var fn = function() {
        if (document.location.hash.indexOf('float') > -1) {

            //not the best place for it, but this still needs to happen:
            var cols = $("tr.supertest:first>td:visible").length;
            $("tr.category>td").attr('colspan', cols);

            if( created ) {
                return floatThead.call(this);
            } else {
                return floatThead.call(this, "reflow");
            }
        }
    };
    return fn;
})(table.floatThead);

i can look more at this later today

kangax commented 8 years ago

Ah... I actually changed it later to floatThead.apply(this, arguments) and was later calling floatThead('destroy') followed by floatThead(). This fixed the issue with table not expanding to the right properly but introduced some sort of a glitch with offset when the table is collapsed (no obsolete shown). Reflow is a good idea!

kangax commented 8 years ago

Great, I tweaked your snippet and it's now working perfectly — https://github.com/kangax/compat-table/commit/24b0982784f1270df4fae8907a77bcef73d59d10

One last thing left (I think) — sorting :)

mkoryak commented 8 years ago

Awesome!

you basically need to reflow every time the dom in the table is modified so that the header can re-align itself. you probably need to do this at the end of the sort function. Another way to do it would be table.triggerHandler('reflow'), if you like that style better.

kangax commented 8 years ago

Reflow doesn't seem to help after sorting. Something is definitely off.

screenshot 2015-12-29 13 15 47

mkoryak commented 8 years ago

ok, i see whats going on here.

I set overflow: hidden on the container that has the floating table, it makes sense to do this 99% of the time, in your case, you can remove it:

.floatThead-container {
   overflow: visible;
}

but this still looks funny when you scroll. we can fix that by passing a top: function(){ return 10 when sorting, else 0 } to the config for the plugin.

but I think a better fix is to simply add an empty cell above the features row (when sorting), and then none of these hacks will be required.

mkoryak commented 8 years ago

actually there is more here. when you sort you are doing something to all trs in the table. when the header is floated, the trs in the header are no longer in your table.

either destroy floatthead before sorting, or change

https://github.com/kangax/compat-table/blob/24b0982784f1270df4fae8907a77bcef73d59d10/master.js#L460

to

table.detach().floatThead('getRowGroups').find('tr').each(function(i, row) {

mkoryak commented 8 years ago

ok, i debugged a little and see one more change you need to make, and perhaps add a bug for me.

return floatThead.call(this, {
  'headerCellSelector': 'tr:last>*:visible'
});
kangax commented 8 years ago

Phew, I think everything is finally fixed, but please double-check!

http://kangax.github.io/compat-table/es6/#float

Thanks for all the help!

mkoryak commented 8 years ago

No problem, thanks for the table, i use it a bunch :)

so one small problem, when you are in sort mode, and you scroll, you lose the % numbers.

try this fix:

return floatThead.call(this, {
  'headerCellSelector': 'tr:last>*:visible',
  'top': function(){
      return $('#sort:checked').length * 15; //account for .num-features sticking above header
  }
});
kangax commented 8 years ago

That one is tricky because there's no background behind % numbers and so even with offset they blend into the rest of the table during scroll.

mkoryak commented 8 years ago

ok how about this, we move the percentages back down into the cells when the headers are floating:

table.on("floatThead", function(evt, isFloating, $container){ 
  $container[isFloating ? 'addClass' : 'removeClass']("floating-header");
});
.floatThead-container.floating-header sup.num-features { 
  top: 0;
}
mkoryak commented 8 years ago

also, i noticed that you added jquery.floatThead._.js to your repo. I suggest using jquery.floatThead.js from a cdn, or at least a minified version from dist. Don't use the slim version since you don't have underscore.

kangax commented 8 years ago

Hm, good points! Would be super awesome if you could send a PR! :)

Sent from my iPhone

On 29 Dec 2015, at 19:56, Misha Koryak notifications@github.com wrote:

also, i noticed that you added jquery.floatThead._.js to your source. I suggest using jquery.floatThead.js from a cdn, or at least a minified version from dist. Don't use the slim version since you don't have underscore.

— Reply to this email directly or view it on GitHub.

mkoryak commented 8 years ago

sure, ill see what I can do tomorrow. btw, noticed another bug, when clicking on a header, that column is not sized properly. a reflow is needed somewhere... ill look tomorrow

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.