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

Quiz review navigation - .thispage

    Details

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

          Attachments

            Activity

            Hide
            kaplangwynne 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
            kaplangwynne 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
            kaplangwynne Michael Gwynne added a comment -
            Show
            kaplangwynne Michael Gwynne added a comment - Created from Forum post https://moodle.org/mod/forum/discuss.php?d=240831&mode=3
            Show
            mkassaei Mahmoud Kassaei added a comment - Fix is on: https://github.com/mkassaei/moodle/compare/mdl_42181?expand=1
            Hide
            timhunt 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
            timhunt 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
            mkassaei 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
            mkassaei 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
            timhunt 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
            timhunt 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
            timhunt Tim Hunt added a comment -

            Just updating workflow state.

            Show
            timhunt Tim Hunt added a comment - Just updating workflow state.
            Hide
            mkassaei 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
            mkassaei 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
            timhunt Tim Hunt added a comment -

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

            Show
            timhunt Tim Hunt added a comment - Great. Thanks Mahmoud. This is ready for integration now. (After the 2.6 release is out.)
            Hide
            marina 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 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            stronk7 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
            stronk7 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
            mkassaei 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
            mkassaei 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
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Integrated to master, 26, 25 and 24 - thanks Mahmoud
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  13/Jan/14