Moodle
  1. Moodle
  2. MDL-27600

Navigation: Should be able to add nodes at other places than end of list

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide

      Test difficulty: EASY

      1. I added a unit test for this; run unit tests for the path: lib/simpletest/testnavigationlib.php (should be green)
      2. Look at existing Moodle pages which use the navigation block. Suggested examples:

      • Course page
      • Module page
        Verify that navigation block still appears as before this patch.
      Show
      Test difficulty: EASY 1. I added a unit test for this; run unit tests for the path: lib/simpletest/testnavigationlib.php (should be green) 2. Look at existing Moodle pages which use the navigation block. Suggested examples: Course page Module page Verify that navigation block still appears as before this patch.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-27600-master
    • Rank:
      17264

      Description

      When updating navigation, such as in the module_extend_settings_navigation function, currently Moodle only lets you add an option at the end.

      For some plugins it is a usability benefit to add options at another position in the list. For example I have a custom module which includes the 'Turn editing on' option. I would like this to be in the first place in the module's setting block, just as the same option is included in the first place of the course settings block.

      I am coding this, will update with details soonish.

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Code done now, checking it before submitting

          Show
          Sam Marshall added a comment - Code done now, checking it before submitting
          Hide
          Sam Marshall added a comment -

          Here is the code change. I think this is a slight improvement to the system and I avoided duplicating code so hopefully it is ok.

          Note: The diff looks maybe slightly clearer with the '--patience' option to git diff (but github view is not bad).

          Show
          Sam Marshall added a comment - Here is the code change. I think this is a slight improvement to the system and I avoided duplicating code so hopefully it is ok. Note: The diff looks maybe slightly clearer with the '--patience' option to git diff (but github view is not bad).
          Hide
          Tim Hunt added a comment -

          I just had a quick look at the code, and it looks reasonable to me. +1

          I will be interested to hear Sam H's comments on whether he things it is a good option to offer. If it is accepted, I can improve the contents of the quiz settings block.

          Show
          Tim Hunt added a comment - I just had a quick look at the code, and it looks reasonable to me. +1 I will be interested to hear Sam H's comments on whether he things it is a good option to offer. If it is accepted, I can improve the contents of the quiz settings block.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've just a quick look over this and it looks good to me - certainly gets my +1.
          I'll take a closer look at it today but I imagine it will be good to go in as part of the next integration.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just a quick look over this and it looks good to me - certainly gets my +1. I'll take a closer look at it today but I imagine it will be good to go in as part of the next integration. Cheers Sam
          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
          Sam Marshall added a comment -

          Rebased, thanks (also - thanks for comments earlier).

          Show
          Sam Marshall added a comment - Rebased, thanks (also - thanks for comments earlier).
          Hide
          Sam Hemelryk added a comment -

          Hi Sam,

          This has been integrated now, there was one minor thing I noticed during review that I fixed up before integration:
          $this->last should always be a reference to the last node that was added as a child.
          I hope it is OK I fixed this up as part of integration - given it was very simple and the 2.1 code freeze is looming this is

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Sam, This has been integrated now, there was one minor thing I noticed during review that I fixed up before integration: $this->last should always be a reference to the last node that was added as a child. I hope it is OK I fixed this up as part of integration - given it was very simple and the 2.1 code freeze is looming this is Cheers Sam
          Hide
          Sam Marshall added a comment -

          Thanks Sam - if you are sure that is how $this->last is supposed to work then yes that fix is good, thank you.

          I had assumed that $this->last was supposed to be a reference to the last item in the list (obviously if you add one in the middle, that doesn't change the last item in the list) rather than the last one added, which is why I did the code that way, but probably I misunderstood it.

          Show
          Sam Marshall added a comment - Thanks Sam - if you are sure that is how $this->last is supposed to work then yes that fix is good, thank you. I had assumed that $this->last was supposed to be a reference to the last item in the list (obviously if you add one in the middle, that doesn't change the last item in the list) rather than the last one added, which is why I did the code that way, but probably I misunderstood it.
          Hide
          Aparup Banerjee added a comment -

          yup this works for me

          Show
          Aparup Banerjee added a comment - yup this works for me
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now part of upstream. Many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This is now part of upstream. Many thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: