Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-25596

Improvements for the navigation blocks JS

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

      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

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            samhemelryk 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
            samhemelryk 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - tiny patch, I will have a look at it after the meeting, thanks
            Show
            samhemelryk Sam Hemelryk added a comment - Ready for peer-review https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25596
            Hide
            andyjdavis 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
            andyjdavis 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
            samhemelryk Sam Hemelryk added a comment -

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

            Cheers
            Sam

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

            LOL re:Netscape

            Show
            samhemelryk Sam Hemelryk added a comment - LOL re:Netscape
            Hide
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            samhemelryk 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
            samhemelryk 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
            andyjdavis 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
            andyjdavis 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk Sam Hemelryk added a comment -

            PULL-80 has been created for the remaining changes

            Show
            samhemelryk Sam Hemelryk added a comment - PULL-80 has been created for the remaining changes
            Hide
            mudrd8mz David Mudrák 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
            mudrd8mz David Mudrák 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
            mudrd8mz David Mudrák 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
            mudrd8mz David Mudrák 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
            samhemelryk 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
            samhemelryk 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  21/Feb/11