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

Use CSS to make mod show/hide icons not look "clickable" in hidden sections

    Details

    • Testing Instructions:
      Hide
      • Add an activity or resource to a section
      • Hide the section
      • Move your mouse over the hide/show icon for the resource, and confirm that it remains as the arrow rather than going to the hand icon
      • Move your mouse over the hide/show icon in a non-hidden section, and confirm that it does change to the hand icon
      Show
      Add an activity or resource to a section Hide the section Move your mouse over the hide/show icon for the resource, and confirm that it remains as the arrow rather than going to the hand icon Move your mouse over the hide/show icon in a non-hidden section, and confirm that it does change to the hand icon
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-37162_master
    • Rank:
      46735

      Description

      Currently, the resource toolbox initialiser (specifically, _setup_for_resource) wastes time by mucking about fetching show/hide icons and then fetching their section ancestor in order to check whether it's hidden, then pointlessly setting the show/hide icon's cursor value to "auto" if the section is hidden. This achieves nothing except wasting time - it's not even doing what it was intended to, since cursor:auto on an a tag (which the show/hide icon is) results in the hand icon anyway!

      This should be done using CSS instead (setting it to cursor:default, which actually achieves the stated goal ("Disable" show/hide icons (change cursor to not look clickable) if section is hidden).

        Activity

        Hide
        Andrew Nicols added a comment -

        Hi Paul,

        This looks good but I did find a case where the cursor style is still set to 'auto' in the JS which overrides the CSS as a result:

        diff --git a/course/yui/toolboxes/toolboxes.js b/course/yui/toolboxes/toolboxes.js
        index 9846f9e..e06a977 100644
        --- a/course/yui/toolboxes/toolboxes.js
        +++ b/course/yui/toolboxes/toolboxes.js
        @@ -699,12 +699,6 @@ YUI.add('moodle-course-toolboxes', function(Y) {
                         if (Y.Array.indexOf(response.resourcestotoggle, activityid) != -1) {
                             this.toggle_hide_resource_ui(button);
                         }
        -
        -                if (value == 0) {
        -                    button.setStyle('cursor', 'auto');
        -                } else {
        -                    button.setStyle('cursor', 'pointer');
        -                }
                     }, this);
                 },
                 toggle_highlight : function(e) {
        

        When you toggle a section's visibility, this code is called, so it only affects the icons until you reload the page.

        Show
        Andrew Nicols added a comment - Hi Paul, This looks good but I did find a case where the cursor style is still set to 'auto' in the JS which overrides the CSS as a result: diff --git a/course/yui/toolboxes/toolboxes.js b/course/yui/toolboxes/toolboxes.js index 9846f9e..e06a977 100644 --- a/course/yui/toolboxes/toolboxes.js +++ b/course/yui/toolboxes/toolboxes.js @@ -699,12 +699,6 @@ YUI.add('moodle-course-toolboxes', function(Y) { if (Y.Array.indexOf(response.resourcestotoggle, activityid) != -1) { this .toggle_hide_resource_ui(button); } - - if (value == 0) { - button.setStyle('cursor', 'auto'); - } else { - button.setStyle('cursor', 'pointer'); - } }, this ); }, toggle_highlight : function(e) { When you toggle a section's visibility, this code is called, so it only affects the icons until you reload the page.
        Hide
        Paul Nicholls added a comment -

        Ah, well spotted. I somehow wasn't a watcher on this ticket, so I only just noticed your comment; I'll have a look at it today.

        Show
        Paul Nicholls added a comment - Ah, well spotted. I somehow wasn't a watcher on this ticket, so I only just noticed your comment; I'll have a look at it today.
        Hide
        Paul Nicholls added a comment -

        I've rebased all three branches, and included Andrew's change.

        Show
        Paul Nicholls added a comment - I've rebased all three branches, and included Andrew's change.
        Hide
        Andrew Nicols added a comment -

        Looks good to me. Submit for integration review when you're ready.

        Show
        Andrew Nicols added a comment - Looks good to me. Submit for integration review when you're ready.
        Hide
        Paul Nicholls added a comment -

        Hi Andrew,
        I don't think I have permission to submit for integration review (if I do, I certainly can't find a way to do so). Are you able to submit it?

        Show
        Paul Nicholls added a comment - Hi Andrew, I don't think I have permission to submit for integration review (if I do, I certainly can't find a way to do so). Are you able to submit it?
        Hide
        Eloy Lafuente (stronk7) 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
        Eloy Lafuente (stronk7) 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
        Dan Poltawski added a comment -

        Thanks Paul! Integrated to master, 24 and 23

        Show
        Dan Poltawski added a comment - Thanks Paul! Integrated to master, 24 and 23
        Hide
        David Monllaó added a comment -

        It passes, it works as expected

        Show
        David Monllaó added a comment - It passes, it works as expected
        Hide
        Rex Lorenzo added a comment -

        Hidden content in hidden sections should be able to be unhidden, please see "MDL-34391 - Activities in hidden topic cannot be toggled visible/hidden using show/hide icon".

        Show
        Rex Lorenzo added a comment - Hidden content in hidden sections should be able to be unhidden, please see " MDL-34391 - Activities in hidden topic cannot be toggled visible/hidden using show/hide icon".
        Hide
        Paul Nicholls added a comment -

        Rex, as per my comment on MDL-34391, this is a performance improvement and not a change in behaviour. No decision seems to have been made in MDL-34391 to change the behaviour - if such a decision is made, the change can be made at least as easily after this has been integrated.

        Show
        Paul Nicholls added a comment - Rex, as per my comment on MDL-34391 , this is a performance improvement and not a change in behaviour. No decision seems to have been made in MDL-34391 to change the behaviour - if such a decision is made, the change can be made at least as easily after this has been integrated.
        Hide
        Dan Poltawski added a comment -

        Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

        Show
        Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: