Moodle
  1. Moodle
  2. MDL-42181

Quiz review navigation - .thispage

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5.2
    • Fix Version/s: 2.4.8, 2.5.4, 2.6.1
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a quiz with a several questions spread over a few pages.
      2. Attempt the quiz, and submit so you are on the review page.
      3. Click 'Show all questions on one page' and check that all the navigation buttons are highlighted (solid border) and when clicking on 'Show one page at a time' only the current button is highlighted.
      Show
      Create a quiz with a several questions spread over a few pages. Attempt the quiz, and submit so you are on the review page. Click 'Show all questions on one page' and check that all the navigation buttons are highlighted (solid border) and when clicking on 'Show one page at a time' only the current button is highlighted.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      When reviewing a quiz, the navigation items representing questions on that page have the class 'thispage' on them.

      When you click 'Show all questions on one page', the 'thispage' class is not applied to every nav item. I would have thought it should be.

      Possibly also include a class along the lines of 'allquestionsononepage' in case the themer wants to remove styling when all pages are shown on the same page.

        Gliffy Diagrams

          Activity

          Hide
          Michael Gwynne added a comment -

          Items i-9 are correctly highlighted (bordered) in the first image.
          I believe all items (i-11) should be highlighted in the second image, but only i-9 are.

          Show
          Michael Gwynne added a comment - Items i-9 are correctly highlighted (bordered) in the first image. I believe all items (i-11) should be highlighted in the second image, but only i-9 are.
          Hide
          Michael Gwynne added a comment -
          Show
          Michael Gwynne added a comment - Created from Forum post https://moodle.org/mod/forum/discuss.php?d=240831&mode=3
          Show
          Mahmoud Kassaei added a comment - Fix is on: https://github.com/mkassaei/moodle/compare/mdl_42181?expand=1
          Hide
          Tim Hunt added a comment -

          Thanks for workign on this Mahmoud. I think this is close to right, but there are some issues:

          1. I think the allquestionsononepage / multipages class should go on the <div class="qn_buttons">. It should not be repeated on each button.

          2. you cannot just call optional_param in the middle of the renderer. It breaks encapsulation badly. $panel knows whether all question are being shown (protected $showall) so the right design is probably to add a new method get_showall_class() to quiz_nav_panel_base that returns allquestionsononepage or multipages based on $this->showall.

          3. Similarly, don't mess around with $thispage in the renderer. Set $button->currentpage correctly in get_question_buttons. That is, if $this->showall is true, then $button->currentpage should always be true.

          4. I think, if you make those changes, then you don't need to change the CSS.

          (Hmm. I don't think get_showall_class is a very good method name. Perhaps get_button_container_class?)

          Show
          Tim Hunt added a comment - Thanks for workign on this Mahmoud. I think this is close to right, but there are some issues: 1. I think the allquestionsononepage / multipages class should go on the <div class="qn_buttons">. It should not be repeated on each button. 2. you cannot just call optional_param in the middle of the renderer. It breaks encapsulation badly. $panel knows whether all question are being shown (protected $showall) so the right design is probably to add a new method get_showall_class() to quiz_nav_panel_base that returns allquestionsononepage or multipages based on $this->showall. 3. Similarly, don't mess around with $thispage in the renderer. Set $button->currentpage correctly in get_question_buttons. That is, if $this->showall is true, then $button->currentpage should always be true. 4. I think, if you make those changes, then you don't need to change the CSS. (Hmm. I don't think get_showall_class is a very good method name. Perhaps get_button_container_class?)
          Hide
          Mahmoud Kassaei added a comment -

          Thanks Tim,
          I have modified the code accordingly and tested it.
          The diff is on:
          https://github.com/mkassaei/moodle/compare/mdl_42181?expand=1

          Show
          Mahmoud Kassaei added a comment - Thanks Tim, I have modified the code accordingly and tested it. The diff is on: https://github.com/mkassaei/moodle/compare/mdl_42181?expand=1
          Hide
          Tim Hunt added a comment -

          The code looks good now. Just a few final things

          1. The comment "Return CSS class name depending on preview settings on quiz navigation" is not very accurate. Can you improve it?
          2. You should re-base the branch so that it is a single commit. (git rebase -i, then squash everything to one commit.)
          3. You should make versions of the fix for MOODLE_25_STABLE and MOODLE_24_STABLE (which you can do with git cherry-pick).
          Show
          Tim Hunt added a comment - The code looks good now. Just a few final things The comment "Return CSS class name depending on preview settings on quiz navigation" is not very accurate. Can you improve it? You should re-base the branch so that it is a single commit. (git rebase -i, then squash everything to one commit.) You should make versions of the fix for MOODLE_25_STABLE and MOODLE_24_STABLE (which you can do with git cherry-pick).
          Hide
          Tim Hunt added a comment -

          Just updating workflow state.

          Show
          Tim Hunt added a comment - Just updating workflow state.
          Hide
          Mahmoud Kassaei added a comment -

          I have cherry-picked the branch MDL-42181 into new branches MDL-42181_24 and MDL-42181_25 and tested the changes on MOODLE_24_STABLE and MOODLE_25_STABLE repectively.

          Here are the diff:
          https://github.com/mkassaei/moodle/compare/master...MDL-42181
          https://github.com/mkassaei/moodle/compare/MOODLE_24_STABLE...MDL-42181_24
          https://github.com/mkassaei/moodle/compare/MOODLE_25_STABLE...MDL-42181_25

          Show
          Mahmoud Kassaei added a comment - I have cherry-picked the branch MDL-42181 into new branches MDL-42181 _24 and MDL-42181 _25 and tested the changes on MOODLE_24_STABLE and MOODLE_25_STABLE repectively. Here are the diff: https://github.com/mkassaei/moodle/compare/master...MDL-42181 https://github.com/mkassaei/moodle/compare/MOODLE_24_STABLE...MDL-42181_24 https://github.com/mkassaei/moodle/compare/MOODLE_25_STABLE...MDL-42181_25
          Hide
          Tim Hunt added a comment -

          Great. Thanks Mahmoud. This is ready for integration now. (After the 2.6 release is out.)

          Show
          Tim Hunt added a comment - Great. Thanks Mahmoud. This is ready for integration now. (After the 2.6 release is out.)
          Hide
          Marina Glancy added a comment -

          The issue looks good but let's hold it for a week then for after 2.6 release like Tim said. We are a little short on testing resources now.

          Sorry I have to make some noise with statuses because of workflow

          Show
          Marina Glancy added a comment - The issue looks good but let's hold it for a week then for after 2.6 release like Tim said. We are a little short on testing resources now. Sorry I have to make some noise with statuses because of workflow
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          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
          Mahmoud Kassaei added a comment -

          Thanks Eloy,
          I have rebased my PULL branches (MDL-42181, MDL-42181_24 and MDL-42181_25) on latest version of master, MOODLE_24_STABLE and MOODLE_25_STABLE respectively.

          Show
          Mahmoud Kassaei added a comment - Thanks Eloy, I have rebased my PULL branches ( MDL-42181 , MDL-42181 _24 and MDL-42181 _25) on latest version of master, MOODLE_24_STABLE and MOODLE_25_STABLE respectively.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 26, 25 and 24 - thanks Mahmoud

          Show
          Dan Poltawski added a comment - Integrated to master, 26, 25 and 24 - thanks Mahmoud
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Mahmoud,

          Works fine for me, I can see border on all navigation buttons while viewing all questions and only on one navigation button while viewing single question.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Mahmoud, Works fine for me, I can see border on all navigation buttons while viewing all questions and only on one navigation button while viewing single question.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ...
          But still, I thank you, for you made me stronger…

          Stronger as the beast that roar in the wild
          Stronger as the storm across the ocean
          Stronger as the diamond that won’t break
          Stronger enough to take all heart aches….

          I thank you my friend, for you made me stronger…

          ---- Juneah Landicho

          Closing as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - ... But still, I thank you, for you made me stronger… Stronger as the beast that roar in the wild Stronger as the storm across the ocean Stronger as the diamond that won’t break Stronger enough to take all heart aches…. I thank you my friend, for you made me stronger… ---- Juneah Landicho Closing as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: