HUE-301. Break Partial Refresh into a stand-alone class

Review Request #1183 - Created Nov. 5, 2010 and submitted

Aaron Newton
old-hue-rw
HUE-301
hue
philip, Thomas
commit c58c8d10b4cb4f9d8c55e99799ee162f23221a2e
Author: Aaron Newton <git@clientcide.com>
Date:   Thu Nov 4 15:11:37 2010 -0700

    HUE-301. Break Partial Refresh into a stand-alone class

:100644 100644 f8d2078... 3ee8bcc... M	apps/beeswax/src/beeswax/templates/watch_wait.mako
:100644 100644 a930f8f... 43aec21... M	apps/jframegallery/src/jframegallery/templates/html-table.treeview.ajax.mako
:100644 100644 61b5381... 836f32e... M	apps/jframegallery/src/jframegallery/templates/html-table.treeview.html
:100644 100644 ec0fe31... 926dd0d... M	apps/jframegallery/src/jframegallery/templates/partial_refresh.mako
:100644 100644 a43e2f1... 52d0e43... M	apps/jobsub/src/jobsub/templates/watch_submission.html
:100644 100644 becddf0... 4b33172... M	desktop/core/static/css/shared.css
:100644 100644 977f354... 821781b... M	desktop/core/static/js/Source/BehaviorFilters/Behavior.HtmlTableRestore.js
:100644 100644 3a7d3c9... 04559c2... M	desktop/core/static/js/Source/CCS/CCS.JBrowser.js
:100644 100644 36f5a33... 7bc9a52... M	desktop/core/static/js/Source/CCS/CCS.JFrame.js
:100644 100644 9b329db... befd64f... M	desktop/core/static/js/Source/JFrameLinkers/CCS.JFrame.LivePath.js
:100644 100644 e343272... a1f7199... M	desktop/core/static/js/Source/JFrameRenderers/CCS.JFrame.PartialRefresh.js
:000000 100644 0000000... 1b35f43... A	desktop/core/static/js/Source/JFrameRenderers/PartialUpdate.js
:100644 100644 c99ea06... 65bdf13... M	desktop/core/static/js/package.yml
:000000 100644 0000000... 0783412... A	ext/thirdparty/js/test-runner/Configuration.js
:000000 100644 0000000... 17204dc... A	ext/thirdparty/js/test-runner/hue/PartialUpdate.js
Wrote some stand alone tests using the mootools test runner. Verified all jframe gallery examples worked. Verified against apps using these things that they work.

Note that this required that stand-alone partials (those not in containers) get a new data property (data-single-partial-id instead of data-partial-id). It also requires that these ids be selectable by the css selector engine, so spaces and most punctuation is now illegal.

If I never work on partial refresh again it will be too soon.
  1. Was this tested in all target browsers?
  2. How often does `_defaultRenderer` get called?
    Does this `filter` function need to be redefined every time?
    
    I'd move it outside into the parent closure or at least inside the `if (options.filter)` block so that it gets redefined less often.
    1. this was moved from elsewhere; I don't need this function anymore so I've updated it per your suggestion so that it's now just a conditional.
  3. I'm not a fan of classNames with underscores.
    
    I have it on my todo list to audit all current classNames and data-* attributes so that we can begin solidifying our conventions.
    
    In general I recommend mixedCaps for classNames, but we should all decide together.
    
    Since this is a refactor and not the original code, it's too late for me to bring up this issue, so I guess I shouldn't have.
    1. this feature is new. I could change it to be orderedPartialRefresh but that would be inconsistent with our other conventions for class names; mixed caps in our class names are at the moment very rare or non-existent... I'm inclined to leave it because of this...
  4. Call it a micro-optimization if you will, but most MooToolers I've talked to prefer to do a `var self = this;` and then rely on closure access to `self` instead of using bind and having the impact of an extra function call for each.
    
    I'd actually recommend something more descriptive than `self` which would also help with the readability of this code and tracing back the multiple binds and many possible resolutions of `this`.
    
    I have a TextMate command that help with this pattern, remind me to hook you up with it.
    1. I've contemplated making this switch (it was an anti-pattern not too long ago in the MooTools codebase). I haven't really yet done it mostly for consistency. In general, I've always tried to follow the conventions outlined in Core, and not until recently did we start eschewing from .bind(). I don't think that this feature should be held up for an overhaul of our binding practices, but I am open to doing a search through the code and making this change. Ultimately I think that the performance savings probably don't merit the effort.
  5. Missing ; before the (
    
    :P
  6. desktop/core/static/js/package.yml (Diff revision 1)
     
     
    No CCS.JFrame namespace?
    1. We've been moving away from CCS.* This thing doesn't know anything about JFrame so it's not on that namespace. Further, in my "make it stand-alone" work I've removed all CCS references.
  7. I'm all for moving this stuff into a new Class. Good stuff™
Loading...