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

      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

        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 Skoda added a comment -

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

          Show
          Petr Skoda 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: