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

Body shrinking #276

Closed
kangax opened this issue Dec 28, 2015 · 22 comments
Closed

Body shrinking #276

kangax opened this issue Dec 28, 2015 · 22 comments
Labels

Comments

@kangax
Copy link

kangax commented Dec 28, 2015

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
Copy link
Owner

mkoryak commented Dec 28, 2015

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
Copy link
Owner

mkoryak commented Dec 28, 2015

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

@mkoryak
Copy link
Owner

mkoryak commented Dec 28, 2015

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
Copy link
Author

kangax commented Dec 29, 2015

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

@kangax
Copy link
Author

kangax commented Dec 29, 2015

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

  • Removed colgroup
  • Changed ~ selector to body one
  • Added floatThead initialization on load and each time unstable/obsolete checkbox is clicked and columns are rerendered

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
Copy link
Author

kangax commented Dec 29, 2015

Firefox (nightly) seems to have a slight shift:

screenshot 2015-12-29 12 11 09

@mkoryak
Copy link
Owner

mkoryak commented Dec 29, 2015

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
Copy link
Author

kangax commented Dec 29, 2015

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
Copy link
Author

kangax commented Dec 29, 2015

Great, I tweaked your snippet and it's now working perfectly — compat-table/compat-table@24b0982

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

@mkoryak
Copy link
Owner

mkoryak commented Dec 29, 2015

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
Copy link
Author

kangax commented Dec 29, 2015

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

screenshot 2015-12-29 13 15 47

@mkoryak
Copy link
Owner

mkoryak commented Dec 29, 2015

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
Copy link
Owner

mkoryak commented Dec 29, 2015

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
Copy link
Owner

mkoryak commented Dec 29, 2015

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
Copy link
Author

kangax commented Dec 29, 2015

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
Copy link
Owner

mkoryak commented Dec 29, 2015

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
Copy link
Author

kangax commented Dec 29, 2015

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
Copy link
Owner

mkoryak commented Dec 30, 2015

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
Copy link
Owner

mkoryak commented Dec 30, 2015

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
Copy link
Author

kangax commented Dec 30, 2015

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
Copy link
Owner

mkoryak commented Dec 30, 2015

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

@mkoryak mkoryak closed this as completed Dec 30, 2015
@mkoryak mkoryak reopened this Dec 30, 2015
@mkoryak mkoryak closed this as completed Jan 2, 2016
@mkoryak mkoryak added the solved label Jan 2, 2016
@lock
Copy link

lock bot commented Dec 10, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants