Moodle
  1. Moodle
  2. MDL-25596

Improvements for the navigation blocks JS

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Navigation
    • Labels:
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15205

      Description

      I've attached a patch here that has reworked the navigation blocks JavaScript.
      The biggest change is that it is now a proper Moodle YUI module.
      As well as that I have made the following changes to code:

      • A loading icon is displayed when a branch is being loaded by AJAX.
      • Fixed a bug where you could trigger multiple AJAX requests by rapidly clicking an unloaded branch.
      • Fixed a bug where empty branches weren't being marked as such after a successful AJAX load.
      • When docked the width of the blocks dock panel is now inspected an increased if required to try avoid horizontal scrolling.
      • Removed the no longer needed inclusion of the YUI2 dom library from the navigation and settings block.
      • Expandable nodes are now passed as JS data allowing the navigation JS to be initialised through block_navigation::get_required_javascript.
      • AJAX is now focused around the branch in question rather than the tree in general.
      • Expansion of branches is now delegated to the tree rather than being an individual event on all branches.
      • Tidied up the code in general removing unneeded-unused parameters.

      This patch requires a little more testing across multiple browsers and will then be ready for review.
      Cheers
      Sam

      1. MDL-25596.20101210.patch
        36 kB
        Sam Hemelryk
      1. arrow.gif
        6 kB
      2. non_expanded_section.png
        104 kB

        Activity

        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        Could someone please review the patch I've just attached for this issue it makes some pretty cool changes to the navigation block (see the issue for complete changes).

        Because git rocks my socks presently you can also review at https://github.com/samhemelryk/moodle/commit/c8502c767c3db0c7578ae5a4ec1e901ca5970051 if you prefer

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, Could someone please review the patch I've just attached for this issue it makes some pretty cool changes to the navigation block (see the issue for complete changes). Because git rocks my socks presently you can also review at https://github.com/samhemelryk/moodle/commit/c8502c767c3db0c7578ae5a4ec1e901ca5970051 if you prefer Cheers Sam
        Hide
        Petr Škoda added a comment -

        tiny patch, I will have a look at it after the meeting, thanks

        Show
        Petr Škoda added a comment - tiny patch, I will have a look at it after the meeting, thanks
        Hide
        Sam Hemelryk added a comment -
        Show
        Sam Hemelryk added a comment - Ready for peer-review https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25596
        Hide
        Andrew Davis added a comment -

        Guess I still have written enough YUI JS as this stuff still makes my head hurt.

        Generally, really well commented

        line 95 of blocks/settings/block_settings.php whitespace. Im told you need a space either side of the => when declaring an array Same on line 543 of lib/navigationlib.php

        Had a bash on the navigation block using Ubuntu and FF 3.6.13, Windows 7 with both IE 8 and Safari 5.0.3. Seems pretty solid. It doesnt work at all in Netscape 6 but I doubt thats an issue worth worrying about.

        Show
        Andrew Davis added a comment - Guess I still have written enough YUI JS as this stuff still makes my head hurt. Generally, really well commented line 95 of blocks/settings/block_settings.php whitespace. Im told you need a space either side of the => when declaring an array Same on line 543 of lib/navigationlib.php Had a bash on the navigation block using Ubuntu and FF 3.6.13, Windows 7 with both IE 8 and Safari 5.0.3. Seems pretty solid. It doesnt work at all in Netscape 6 but I doubt thats an issue worth worrying about.
        Hide
        Sam Hemelryk added a comment -

        Cool thanks Andrew,
        I've cleaned up the whitespace and will create a pull request now

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Cool thanks Andrew, I've cleaned up the whitespace and will create a pull request now Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        LOL re:Netscape

        Show
        Sam Hemelryk added a comment - LOL re:Netscape
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Reopening this as far as I've get wrong results (unless I'm doing something wrong) when testing PULL-50.

        Basic use test failing:

        • Purge all caches (just to be sure).
        • Go to course page with 3-4 sections, each one having any number or activities.
        • Clicking in the section, shows the ajax spinning wheel.
        • BUT, the activities aren't shown at all.

        Exactly the same test into current upstream works ok (without the spinning wheel, but works).

        Same results both with Safari & Firefox. Added sample screenshot. Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Reopening this as far as I've get wrong results (unless I'm doing something wrong) when testing PULL-50. Basic use test failing: Purge all caches (just to be sure). Go to course page with 3-4 sections, each one having any number or activities. Clicking in the section, shows the ajax spinning wheel. BUT, the activities aren't shown at all. Exactly the same test into current upstream works ok (without the spinning wheel, but works). Same results both with Safari & Firefox. Added sample screenshot. Ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Uhm...just detected I had the "Generate navigation for the following" set to "Categories, courses and course structures" but not for activities, grrr. So the block was "doing ok" as far as it was configured that way.

        Anyway... I think the expand/collapse "triangles" shouldn't be shown in the "final" items of the tree and more yet, no AJAX should be executed if there is nothing to show. It really leads to confusion IMO.

        So I keep this open in order to apply those improvements... sorry for forgetting about the setting, at least it was useful for testing.

        Thanks and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Uhm...just detected I had the "Generate navigation for the following" set to "Categories, courses and course structures" but not for activities, grrr. So the block was "doing ok" as far as it was configured that way. Anyway... I think the expand/collapse "triangles" shouldn't be shown in the "final" items of the tree and more yet, no AJAX should be executed if there is nothing to show. It really leads to confusion IMO. So I keep this open in order to apply those improvements... sorry for forgetting about the setting, at least it was useful for testing. Thanks and ciao
        Hide
        Sam Hemelryk added a comment -

        Cool thanks Eloy,

        I've created a fix for this at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25596
        Andrew could you please peer-review it for me.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Cool thanks Eloy, I've created a fix for this at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25596 Andrew could you please peer-review it for me. Cheers Sam
        Hide
        Andrew Davis added a comment -

        Something slightly odd I noticed. The arrow you need to click to expand a glossary activity is displayed under the glossary name. Attaching a screenshot demonstrating it. Using the theme "Standard".

        Show
        Andrew Davis added a comment - Something slightly odd I noticed. The arrow you need to click to expand a glossary activity is displayed under the glossary name. Attaching a screenshot demonstrating it. Using the theme "Standard".
        Hide
        Sam Hemelryk added a comment -

        Cool thanks for checking it out Andrew.
        Regarding the navigation bug I've created MDL-25878 as its a separate issue and requires more then minor changes... will get it included in the next sprint.
        Cheers
        SAm

        Show
        Sam Hemelryk added a comment - Cool thanks for checking it out Andrew. Regarding the navigation bug I've created MDL-25878 as its a separate issue and requires more then minor changes... will get it included in the next sprint. Cheers SAm
        Hide
        Sam Hemelryk added a comment -

        PULL-80 has been created for the remaining changes

        Show
        Sam Hemelryk added a comment - PULL-80 has been created for the remaining changes
        Hide
        David Mudrak added a comment -

        When upgrading to the next master weekly (that is today's master from integration.git), I am getting

        Notice: Trying to get property of non-object in .../blocks/navigation/block_navigation.php on line 115
        
        Show
        David Mudrak added a comment - When upgrading to the next master weekly (that is today's master from integration.git), I am getting Notice: Trying to get property of non-object in .../blocks/navigation/block_navigation.php on line 115
        Hide
        David Mudrak added a comment -

        The Notice above got fixed by re-saving Navigation block settings. However, at Configuring a Navigation block page, I am getting

        
        Notice: Trying to get property of non-object in /home/mudrd8mz/public_html/moodle20/lib/accesslib.php on line 2360
        
        Notice: Trying to get property of non-object in /home/mudrd8mz/public_html/moodle20/lib/accesslib.php on line 2364
        

        not sure if it is related or not...

        Show
        David Mudrak added a comment - The Notice above got fixed by re-saving Navigation block settings. However, at Configuring a Navigation block page, I am getting Notice: Trying to get property of non-object in /home/mudrd8mz/public_html/moodle20/lib/accesslib.php on line 2360 Notice: Trying to get property of non-object in /home/mudrd8mz/public_html/moodle20/lib/accesslib.php on line 2364 not sure if it is related or not...
        Hide
        Sam Hemelryk added a comment -

        Hi David,

        Thanks for the finding, I've created a PULL-107 to fix this issue.
        I'll talk to Petr today when he comes online and find out whether he will integrate PULL-107 or revert PULL-80.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi David, Thanks for the finding, I've created a PULL-107 to fix this issue. I'll talk to Petr today when he comes online and find out whether he will integrate PULL-107 or revert PULL-80. Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Just for the records, PULL-107 has been also integrated / tested / committed upstream. So everything is ok now. Thanks David / Sam / Petr.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Just for the records, PULL-107 has been also integrated / tested / committed upstream. So everything is ok now. Thanks David / Sam / Petr. Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: