Moodle
  1. Moodle
  2. MDL-35672 META JavaScript performance issues
  3. MDL-36287

navigation.js uses node.on when it could use delegation for wire function

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      There should be no visible changes, this is a performance issue.

      1. Play with the navigation.
      2. Expand things by AJAX.
      3. Try both mouse and keyboard navigation.
      Show
      There should be no visible changes, this is a performance issue. Play with the navigation. Expand things by AJAX. Try both mouse and keyboard navigation.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-36287-m25
    • Rank:
      45074

      Description

      I think that this is another candidate for event delegation rather than an Node.on.
      The wire function is called for each expandable branch and just sets up calls to ajaxLoad if the node is expandable. It should be possible to determine whether a node is expandable in the ajaxLoad function and instead to delegate to all tree items.

      I'm not familiar enough with the code to do it right now, and I know that you're already working in this area Sam so I point the finger of doom at you

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Thanks for creating the issue Andrew.

          I'll have a quick look at it right now. If its an easy to implement improvement then I'll make it right now, otherwise it will be added to my list.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for creating the issue Andrew. I'll have a quick look at it right now. If its an easy to implement improvement then I'll make it right now, otherwise it will be added to my list. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Ok I've a solution for this. Was pretty easy.
          No doubt it could be further optimised but I am still hoping to redesign the whole thing one day.

          Show
          Sam Hemelryk added a comment - Ok I've a solution for this. Was pretty easy. No doubt it could be further optimised but I am still hoping to redesign the whole thing one day.
          Hide
          Sam Hemelryk added a comment -

          Up for peer-review now.

          Show
          Sam Hemelryk added a comment - Up for peer-review now.
          Hide
          Andrew Nicols added a comment -

          [N] Syntax
          [Y] Output
          [N] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [Y] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Syntax:

          I'm not keen on the use of the one-line anonymous function for the delegates and in this case there should be no need to use them in any case. You don't need to pass the this object as an argument. If you use a context of this in the delegate (the TREE object), then the event will contain a currentTarget which is the clicked link and is then accessible in the argument supplied to the function (http://yuilibrary.com/yui/docs/api/classes/YUI.html#method_delegate):

          // The delegations.
          somefunction: function(event) {
              Y.delegate('click', this.fire_branch_action, node.one('.block_tree'), '.tree_item.branch[data-expandable]', this);
              Y.delegate('actionkey', this.fire_branch_action, node.one('.block_tree'), '.tree_item.branch[data-expandable]', this);
          },
          
          fire_branch_action: function(event) {
              var id = event.currentTarget.getAttribute('id');
              var branch = this.branches[id];
              branch.ajaxLoad(event);
          },  
          

          Whitespace

          I think we were talking about having no space before the colon in a definition:

          fire_branch_section : function(event) {
          }
          

          Otherwise this looks fine to me.

          Show
          Andrew Nicols added a comment - [N] Syntax [Y] Output [N] Whitespace [-] Language [-] Databases [Y] Testing [Y] Security [-] Documentation [Y] Git [Y] Sanity check Syntax: I'm not keen on the use of the one-line anonymous function for the delegates and in this case there should be no need to use them in any case. You don't need to pass the this object as an argument. If you use a context of this in the delegate (the TREE object), then the event will contain a currentTarget which is the clicked link and is then accessible in the argument supplied to the function ( http://yuilibrary.com/yui/docs/api/classes/YUI.html#method_delegate): // The delegations. somefunction: function(event) { Y.delegate('click', this .fire_branch_action, node.one('.block_tree'), '.tree_item.branch[data-expandable]', this ); Y.delegate('actionkey', this .fire_branch_action, node.one('.block_tree'), '.tree_item.branch[data-expandable]', this ); }, fire_branch_action: function(event) { var id = event.currentTarget.getAttribute('id'); var branch = this .branches[id]; branch.ajaxLoad(event); }, Whitespace I think we were talking about having no space before the colon in a definition: fire_branch_section : function(event) { } Otherwise this looks fine to me.
          Hide
          Sam Hemelryk added a comment -

          Thanks for looking at that Andrew.

          I've tidied up the delegation to use context (thanks for picking that up I must've had a real blonde moment).

          This if up for integration now.

          Show
          Sam Hemelryk added a comment - Thanks for looking at that Andrew. I've tidied up the delegation to use context (thanks for picking that up I must've had a real blonde moment). This if up for integration now.
          Hide
          Damyon Wiese added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          Thanks!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
          Hide
          Damyon Wiese added a comment -

          Thanks Sam.

          The change makes sense to me and I did some testing and did not find any problems. Also - I ran the behat tests and they still pass.

          Integrated to master.

          Show
          Damyon Wiese added a comment - Thanks Sam. The change makes sense to me and I did some testing and did not find any problems. Also - I ran the behat tests and they still pass. Integrated to master.
          Hide
          Mark Nelson added a comment -

          Works as expected, passing.

          Show
          Mark Nelson added a comment - Works as expected, passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Because

          A
          MARVELOUS
          A       U
          Z  YOU  P
          I  ARE  E
          N  PPL  R
          G       B
            TNKS! 
          

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: