tableheader has several problems:

  • complicated computation during the onscroll event
  • hard to extend for contrib
  • messy HTML

What I'd like to see is:

  1. No computation during scroll, listen to a custom event for updating the offset value
  2. initialize tableheader on first scroll, not on page load.
  3. Offload all offset computation to the modules adding the elements, (toolbar)
  4. use data- attributes to store and get offsets
  5. Only initialize tableheader for table longer than half screen space available (or something), tableheader is not useful for small tables (like the attached files table in issues…)
  6. (bonus) try to get rid of the extra table by playing with the original <thead>

With that we should be able to remove to reduce tableheader footprint quite a lot.

CommentFileSizeAuthor
#16 no_sticky_until_scroll.png59.12 KBkiphaas7
#16 old_header_visible_under_sticky.png65 KBkiphaas7
#13 core-js-rewrite-tableheader-1574738-13.patch16.75 KBnod_
#4 core-js-rewrite-tableheader-4.patch14 KBnod_
#3 core-js-rewrite-tableheader-3.patch13.66 KBnod_
#2 core-js-rewrite-tableheader-2.patch13.76 KBnod_
#1 core-js-rewrite-tableheader.patch13.69 KBnod_

Comments

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Active » Needs work
StatusFileSize
new core-js-rewrite-tableheader.patch13.69 KB

Ok first try.

It's pretty different style of code but that's what it takes to clean it up. tableheader is a library, take a look at yui source, openlayers and co to see how it's usually done.

works IE8+ and the rest, it could actually work on IE7, didn't want to go to the trouble of testing.

This superseeds #1417378: Remove eval() from tableHeader JavaScript. Profiling this rewrite give a significantly better perf than the current code (and about twice better than the previous patch).

Should be commented enough. It's easier to read the patched file, pretty much everything changed. I've put a good number of named function because profiling is useless otherwise.

(edit) I've fixed a super annoying 1-2px offset that was driving me crazy too.

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-rewrite-tableheader-2.patch13.76 KB

forgot 2 lines

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-rewrite-tableheader-3.patch13.66 KB

small tweak to for mobile positioning onscroll

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-rewrite-tableheader-4.patch14 KB

We should introduce throttle and debounce for scroll and resize events respectively. http://benalman.com/projects/jquery-throttle-debounce-plugin/ as well as for autocomplete.

Forgot about the toolbar toggle button. Fixed.

andypost’s picture

andypost
he/him
Primary language Russian
commented

does not woks in overlay
EDIT: IE9(8) and ff14a2 works fine without overlay

nod_’s picture

nod_
Primary language French
Location Lille
commented

indeed, have not changed anything for overlay. Just waiting for some feedback on this before finishing it.

(edit) It's no big trouble to make overlay work, just got lazy towards the end.

kiphaas7’s picture

kiphaas7 commented

Slightly offtopic: saw the change from .once() to .one(). Since this was already in jQuery 1.1, why have we (by we I mean drupal) been using .once() exclusively in the past? There must have been a good reason... Drupal 6 already came with jQuery 1.2.

+ for (var i = 0, il = tables.length; i < il; i += 1) {
Not as a complaint, but genuinely interested: why are you using i += 1 instead if i++?

+function tableHeaderReizeHandler(e) {
Why global functions? Backwards compatibility?

+ attach: function(context, settings) {
No de-attach/unattach-ish property?

Big +1 for .debounce() and .throttle() in core as a library, incredibly useful, and very small size.

It's pretty different style of code but that's what it takes to clean it up. tableheader is a library, take a look at yui source, openlayers and co to see how it's usually done.

I've not been following core issues for some months, but is that something that was already agreed upon for D8 js? i.e. use yui source/openlayers and co codingstyle?

nod_’s picture

nod_
Primary language French
Location Lille
commented

.once and .one don't do the same thing. For drupal use, .one won't work, think of new AJAX content. .one would have been triggered with the old elements not the new ones. Same thing if you delegate.

i += 1, see jslint/hint configuration options. We decided not to go with that later on, it was unclear at the time of the patch though. should be i++ to be consistent with the rest of core.

they are not global functions, they are in the closure scope. Naming them helps with debugging/profiling so it's better than anonymous function for that purpose and don't really hinder readability, for me it actually helps.

detach: totally right, missed that part. it should be done properly :)

Still nothing deceided on that level about code style.

:)

kiphaas7’s picture

kiphaas7 commented

they are not global functions, they are in the closure scope. Naming them helps with debugging/profiling so it's better than anonymous function for that purpose and don't really hinder readability, for me it actually helps.

Ouch, can't believe I missed that. :)

Thanks for the update!

kiphaas7’s picture

kiphaas7 commented

(also needs a reroll, doesn't apply against toolbar.js anymore)

kiphaas7’s picture

kiphaas7 commented

Sorry for the third post in a row, but found this typo:

function tableHeaderReizeHandler(e) {

ReizeHandler -> ResizeHandler. Typo is consistent (e.g. you named it ReizeHandler everywhere), probably why you didn't notice it.

nod_’s picture

nod_
Primary language French
Location Lille
commented

It's all constructive feedback, don't worry. Nice to know someone is reviewing :) Hopefully I'll have some time to reroll soon.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new core-js-rewrite-tableheader-1574738-13.patch16.75 KB

Closed #1728122: Sticky Table header tables don't inherit parent table's outer width and #1724534: Sticky Table header rows incorrectly sized if browser resized between detachments since the patch fixes both issues.

Reroll of the patch, overlay works too.

lorique’s picture

lorique commented
Status: Needs review » Reviewed & tested by the community

Test OK. Code is sane.

The tableheader has been prototyped correctly, and there is no longer any unnecessary computing going on during scrolling.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

kiphaas7’s picture

kiphaas7 commented
Status: Fixed » Active
StatusFileSize
new old_header_visible_under_sticky.png65 KB
new no_sticky_until_scroll.png59.12 KB

Think this was all introduced in this patch, therefore re-opening.

See screenshots:

  1. Original header is visible under sticky header (setting a z-index fixes this, but that might open a whole new can of worms
  2. If for some reason you load the page halfway a table, the sticky table will only show up upon the first scroll. Maybe trigger scroll once on pageload if the table is in the viewport?
kiphaas7’s picture

kiphaas7 commented

So I talked a little bit with nod_ about this, and he said the sticky table creation upon scroll is on purpose, to delay execution as much as possible.

A middleground might be to execute tableHeaderInitHandler unconditionally on attach. Move the 'on scroll' bind to createSticky, after which we can test for checkStickyVisible: if it returns true, trigger scroll on the table.

This way the actual creation and insertion of the sticky table, which I think is the most costly operation, is still delayed if the table is not visible.

If that sounds good, I'll try to whip up a patch.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Active » Fixed

No but seriously, make a new bug report please.

kiphaas7’s picture

kiphaas7 commented

#1764912: Fix regressions and further improve tableheader.js

Automatically closed -- issue fixed for 2 weeks with no activity.