Posted by nod_ on May 12, 2012 at 10:51pm
8 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | javascript |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | frontend performance, WPO |
Issue Summary
tableheader has several problems:
- complicated computation during the onscroll event
- hard to extend for contrib
- messy HTML
What I'd like to see is:
- No computation during scroll, listen to a custom event for updating the offset value
- initialize tableheader on first scroll, not on page load.
- Offload all offset computation to the modules adding the elements, (toolbar)
- use data- attributes to store and get offsets
- 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…)
- (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.
Comments
#1
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.
#2
forgot 2 lines
#3
small tweak to for mobile positioning onscroll
#4
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.
#5
does not woks in overlay
EDIT: IE9(8) and ff14a2 works fine without overlay
#6
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.
#7
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.
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?
#8
.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.
:)
#9
Ouch, can't believe I missed that. :)
Thanks for the update!
#10
(also needs a reroll, doesn't apply against toolbar.js anymore)
#11
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.
#12
It's all constructive feedback, don't worry. Nice to know someone is reviewing :) Hopefully I'll have some time to reroll soon.
#13
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.
#14
Test OK. Code is sane.
The tableheader has been prototyped correctly, and there is no longer any unnecessary computing going on during scrolling.
#15
Committed to 8.x. Thanks.
#16
Think this was all introduced in this patch, therefore re-opening.
See screenshots:
#17
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
tableHeaderInitHandlerunconditionally on attach. Move the 'on scroll' bind tocreateSticky, after which we can test forcheckStickyVisible: 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.
#18
No but seriously, make a new bug report please.
#19
#1764912: Fix regressions and further improve tableheader.js
#20
Automatically closed -- issue fixed for 2 weeks with no activity.