Details

    • Testing Instructions:
      Hide

      Testing should be completed with the JS console open in at least IE/Firefox/Chrome:

      • Ensure that you're using the Standard theme
      • Ensure that the doctonewwindow setting is off
      • Open a course
      • Turn editing on
      • At the bottom of the page, click the 'Moodle docs for this page' button
        • Confirm that the page opens in the current window
      • Return to the course
      • Open the Activity Chooser
      • Select an activity
      • Scroll to the bottom of its help
      • Click the 'More help' button
        • Confirm that the page opens as a popup
      • Turn the 'doctonewwindow' setting on
      • Refresh the course page
      • At the bottom of the page, click the 'Moodle docs for this page' button
        • Confirm that the page opens as a popup
      • Open the Activity Chooser again
      • Find and select a 'More help' button
        • Confirm that the page opens as a popup
      Show
      Testing should be completed with the JS console open in at least IE/Firefox/Chrome: Ensure that you're using the Standard theme Ensure that the doctonewwindow setting is off Open a course Turn editing on At the bottom of the page, click the 'Moodle docs for this page' button Confirm that the page opens in the current window Return to the course Open the Activity Chooser Select an activity Scroll to the bottom of its help Click the 'More help' button Confirm that the page opens as a popup Turn the 'doctonewwindow' setting on Refresh the course page At the bottom of the page, click the 'Moodle docs for this page' button Confirm that the page opens as a popup Open the Activity Chooser again Find and select a 'More help' button Confirm that the page opens as a popup
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35836-master
    • Rank:
      44598

      Description

      The doctonewwindow setting alter Moodle help links such that they open in a new popup. It appears as though each link is made using an on click event, which we know to be pretty inefficient.

      It should be relatively easy to modify them such that event delegation is used on all such help links. The links that are opened are the same link as on the anchor.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Raising priority to Major - this is disabled in default installations.

          Show
          Andrew Nicols added a comment - Raising priority to Major - this is disabled in default installations.
          Hide
          Andrew Nicols added a comment -

          This changes the doctonewwindow check to add a new class to the hyperlink instead of generating an individual onclick event.
          Additionally, we always run the setup part of this code (which just delegates any a.helplinkpopup to run a function) so that it can be used at any time, even if doctonewwindow is disabled. We then make use of this in the module chooser which forced a popup when clicking help regardless of the setting of doctonewwindow.

          Show
          Andrew Nicols added a comment - This changes the doctonewwindow check to add a new class to the hyperlink instead of generating an individual onclick event. Additionally, we always run the setup part of this code (which just delegates any a.helplinkpopup to run a function) so that it can be used at any time, even if doctonewwindow is disabled. We then make use of this in the module chooser which forced a popup when clicking help regardless of the setting of doctonewwindow.
          Hide
          Andrew Nicols added a comment -

          This should really help those users with doctonewwindow enabled. It should also increase performance of the page with the activity chooser (there were onclick events for each activities help).

          Longer term, it would be good to look at a more generic commoneventshandler YUI module to replace this and handle standard popups like this without being directly related to the help JS.

          This will need to be backported for 2.3 - I'll create a branch once Peer Review has taken place.

          Show
          Andrew Nicols added a comment - This should really help those users with doctonewwindow enabled. It should also increase performance of the page with the activity chooser (there were onclick events for each activities help). Longer term, it would be good to look at a more generic commoneventshandler YUI module to replace this and handle standard popups like this without being directly related to the help JS. This will need to be backported for 2.3 - I'll create a branch once Peer Review has taken place.
          Hide
          Paul Nicholls added a comment -

          Looks good to me - works as expected, no errors in JS console.

          Show
          Paul Nicholls added a comment - Looks good to me - works as expected, no errors in JS console.
          Hide
          Rajesh Taneja added a comment -

          Hello Andrew,

          Patch looks good, although you might want to consider adding class to helplink by default rather then from JS.

          • As we are doing this for performance, it might be nice to avoid any dom change (adding helplinkpopup class). Might be nice to alter doc_link api to accept forcepopup as optional value and make forcepopup true in get_module_metadata.
          Show
          Rajesh Taneja added a comment - Hello Andrew, Patch looks good, although you might want to consider adding class to helplink by default rather then from JS. As we are doing this for performance, it might be nice to avoid any dom change (adding helplinkpopup class). Might be nice to alter doc_link api to accept forcepopup as optional value and make forcepopup true in get_module_metadata.
          Hide
          Andrew Nicols added a comment -

          Thanks Rajesh,

          That's a great idea - I've made the change as per your suggestion.

          I've rebased on weeklies.

          Submitting for Integration review.

          Show
          Andrew Nicols added a comment - Thanks Rajesh, That's a great idea - I've made the change as per your suggestion. I've rebased on weeklies. Submitting for Integration review.
          Hide
          Aparup Banerjee 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.

          TIA and ciao

          Show
          Aparup Banerjee 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. TIA and ciao
          Hide
          Andrew Nicols added a comment -

          Rebased on latest integration/master

          Show
          Andrew Nicols added a comment - Rebased on latest integration/master
          Hide
          Dan Poltawski added a comment - - edited

          Integrated to 23 and master, thanks

          Show
          Dan Poltawski added a comment - - edited Integrated to 23 and master, thanks
          Hide
          Adrian Greeve added a comment -

          Tested on the 2.3 and master integration branches on firefox, chrome and IE.
          No errors were found while testing.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the 2.3 and master integration branches on firefox, chrome and IE. No errors were found while testing. Test passed.
          Hide
          Dan Poltawski added a comment -

          Hurray!

          You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

          Show
          Dan Poltawski added a comment - Hurray! You did it, congratulations! You have on Mojito credit to redeem after the release of Moodle 2.4

            People

            • Votes:
              10 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: