Details

    • Testing Instructions:
      Hide

      1. Create a quiz, add two questions to it.
      2. Login as a student and attempt the quiz.
      3. Answer one of the questions and click 'Next'.
      4. In the table 'Summary of attempt', make sure that the left column of question numbers uses the tag <th>
      5. Visit other pages using html_writer::table() and make sure no error appears

      • Like Settings > Site administration > Plugins > Filters > Manage filters
      • Or Course administration > Grades
      Show
      1. Create a quiz, add two questions to it. 2. Login as a student and attempt the quiz. 3. Answer one of the questions and click 'Next'. 4. In the table 'Summary of attempt', make sure that the left column of question numbers uses the tag <th> 5. Visit other pages using html_writer::table() and make sure no error appears Like Settings > Site administration > Plugins > Filters > Manage filters Or Course administration > Grades
    • Affected Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30886-master
    • Rank:
      33895

      Description

      In the quiz summary, when about to complete a quiz where it tells users what questions are still unanswered, , the question numbers should be table cell headings.

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          Note to reviewer: I have found that if you pass a row (as an array) to html_writer::table() it considers that the cells are strings, which created a bug in my case. I could have avoided the fix by having the rows and cells as objects, but I thought that it was an unnecessary change within the Quiz itself. I am note sure whether I could fix this table-related bug within this patch or not.

          Waiting for your feedback!

          Show
          Frédéric Massart added a comment - Note to reviewer: I have found that if you pass a row (as an array) to html_writer::table() it considers that the cells are strings, which created a bug in my case. I could have avoided the fix by having the rows and cells as objects, but I thought that it was an unnecessary change within the Quiz itself. I am note sure whether I could fix this table-related bug within this patch or not. Waiting for your feedback!
          Hide
          Dan Poltawski added a comment -

          Since Tim is component maintainer of Quiz, I think it is best he reviews this change, so assignign to him for peer review.

          Show
          Dan Poltawski added a comment - Since Tim is component maintainer of Quiz, I think it is best he reviews this change, so assignign to him for peer review.
          Hide
          Tim Hunt added a comment -

          I have two concerns in principle about this change:

          1. Should the question numbers really be table header cells? They make sense to me as values in the question number column.

          2. This patch will break many themes that use a different background for table headers. You acknowledge that the change is not backwards-compatible with the new

          {font-weight: normal;}

          style rule, but font-weight is not the only thing that might change between td and th, so I am very reluctant for this go to into stable branches.

          I think these issues need to be discussed before we go further.

          In terms of the details of the code:

          3. I like the change in outputcomponents.php.

          4. The patch could be improved by moving the $row = array($cell, $attemptobj->get_question_status($slot, $displayoptions->correctness)); line outside the if statement, since it would eliminate one line of duplication.

          Show
          Tim Hunt added a comment - I have two concerns in principle about this change: 1. Should the question numbers really be table header cells? They make sense to me as values in the question number column. 2. This patch will break many themes that use a different background for table headers. You acknowledge that the change is not backwards-compatible with the new {font-weight: normal;} style rule, but font-weight is not the only thing that might change between td and th, so I am very reluctant for this go to into stable branches. I think these issues need to be discussed before we go further. In terms of the details of the code: 3. I like the change in outputcomponents.php. 4. The patch could be improved by moving the $row = array($cell, $attemptobj->get_question_status($slot, $displayoptions->correctness)); line outside the if statement, since it would eliminate one line of duplication.
          Hide
          Greg Kraus added a comment -

          This change is not critical. I just looked at how it is implemented in 2.3 and it's quite usable as is. While it could be argued that all of the question numbers should be headings, since there are only 2 columns, it's not really that big of a deal.

          Show
          Greg Kraus added a comment - This change is not critical. I just looked at how it is implemented in 2.3 and it's quite usable as is. While it could be argued that all of the question numbers should be headings, since there are only 2 columns, it's not really that big of a deal.
          Hide
          Tim Hunt added a comment -

          Given what has been said so far, I am inclined to say that we should Wont-fix this.

          However, I like the change to lib/outputcomponents.php, and I think we should salvage that change.

          Show
          Tim Hunt added a comment - Given what has been said so far, I am inclined to say that we should Wont-fix this. However, I like the change to lib/outputcomponents.php, and I think we should salvage that change.
          Hide
          Frédéric Massart added a comment -

          Hi Tim,

          how do you want to salvage the fix? I have created 3 branches based on those ones, with the same issue number. Should we use them or create a new issue?

          The master one: https://github.com/FMCorz/moodle/compare/MDL-30886-master-salvation

          Show
          Frédéric Massart added a comment - Hi Tim, how do you want to salvage the fix? I have created 3 branches based on those ones, with the same issue number. Should we use them or create a new issue? The master one: https://github.com/FMCorz/moodle/compare/MDL-30886-master-salvation
          Hide
          Tim Hunt added a comment -

          I guess the best thing is to create a new issues for fixing outputlib.php, and close this issue as won'tfix. That will leave the clearest record of what was decided. (The alternative is just to massively edit this bug, but that is lazy.)

          I tell you what, I could make the new tracker issue, and you could then edit your commit comments and re-submit for integration. Does that sound like a fair division of labour?

          Show
          Tim Hunt added a comment - I guess the best thing is to create a new issues for fixing outputlib.php, and close this issue as won'tfix. That will leave the clearest record of what was decided. (The alternative is just to massively edit this bug, but that is lazy.) I tell you what, I could make the new tracker issue, and you could then edit your commit comments and re-submit for integration. Does that sound like a fair division of labour?
          Hide
          Frédéric Massart added a comment -

          Sounds good

          Show
          Frédéric Massart added a comment - Sounds good
          Hide
          Tim Hunt added a comment -

          MDL-33814 all ready for you.

          Show
          Tim Hunt added a comment - MDL-33814 all ready for you.
          Hide
          Frédéric Massart added a comment -

          Closing this issue

          Show
          Frédéric Massart added a comment - Closing this issue

            People

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

              Dates

              • Created:
                Updated:
                Resolved: