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

User pictures in Feedback module squeezed to 5px

    Details

    • Testing Instructions:
      Hide

      In a course with users, go to the the Feedback module.

      • Set "Record user names' to non-anonymous
      • Add a multiple-choice question
      • Answer with a few students
      • As admin in tab "Analysis"
        • expected: colored bars are height: 5px.
      • in tab "Show responses" and "Show non-respondents"
        • expected: user pictures are not height 5px.
      Show
      In a course with users, go to the the Feedback module. Set "Record user names' to non-anonymous Add a multiple-choice question Answer with a few students As admin in tab "Analysis" expected: colored bars are height: 5px. in tab "Show responses" and "Show non-respondents" expected: user pictures are not height 5px.
    • Affected Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull from Repository:
    • Pull 2.7 Branch:
    • Pull Master Branch:
      MDL-46063_master

      Description

      User pictures in Feedback module appear squeezed to 5px height in the "Show responses" and "Show non-respondents" tabs (mod/feedback/show_entries.php, mod/feedback/show_nonrespondents.php).

      This is a side effect of setting mod_feedback analysis bar heights to 5px in "Analysis" tab (mod/feedback/analysis.php), introduced in MDL-42711.

      Here's the CSS-definition in theme/bootstrapbase/less/moodle/modules.less:

      .path-mod-feedback .generalbox div table tbody img {
          height: 5px; // we should remove the bar height specification in core
      }
      

      A more exact definition would help avoid this kind of misinterpretations.
      Something like this would work better:

      #page-mod-feedback-analysis img.feedback_bar_image {
          height: 5px; // we should remove the bar height specification in core
      }
      

      The following lines can probably be removed from mod/feedback/styles.css if they are not required by other themes.

      div img.feedback_bar_image {
          height: 10px;
      }
      

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for reporting this. Someone had already mentioned this problem as part of another issue (MDL-45952), but it's good to have this issue in isolation.

              Show
              salvetore Michael de Raadt added a comment - Thanks for reporting this. Someone had already mentioned this problem as part of another issue ( MDL-45952 ), but it's good to have this issue in isolation.
              Hide
              salvetore Michael de Raadt added a comment -

              Aparup Banerjee: As you were involved in the related issue, I thought you might be interested in this one.

              Show
              salvetore Michael de Raadt added a comment - Aparup Banerjee : As you were involved in the related issue, I thought you might be interested in this one.
              Hide
              nebgor Aparup Banerjee added a comment -

              thanks for the report!
              +1 suggested solution, feel free to add a patch/github with your css and less fix!

              i'd recommend the legacy mod/feedback/styles.css change too but that depends on the integrator's BC checks. its probably not as fulfilling as the main fix.

              Show
              nebgor Aparup Banerjee added a comment - thanks for the report! +1 suggested solution, feel free to add a patch/github with your css and less fix! i'd recommend the legacy mod/feedback/styles.css change too but that depends on the integrator's BC checks. its probably not as fulfilling as the main fix.
              Hide
              schach Heiko Schach added a comment -

              OK, I'm going to prepare a patch on Github.
              I understand it's bad practice to use IDs for CSS selectors instead of classes.
              So I slightly modified the fix.

              In theme/bootstrapbase/less/moodle/modules.less:

              .path-mod-feedback .feedback_bar_image {
                  height: 5px; // we should remove the bar height specification in core
              }
              

              Show
              schach Heiko Schach added a comment - OK, I'm going to prepare a patch on Github. I understand it's bad practice to use IDs for CSS selectors instead of classes. So I slightly modified the fix. In theme/bootstrapbase/less/moodle/modules.less: .path-mod-feedback .feedback_bar_image { height: 5px; // we should remove the bar height specification in core }
              Hide
              grabs Andreas Grabs added a comment -

              Hi Heiko,

              I am working on moving the nonrespondents list into a mform (MDL-40519). Therefore I've made an example commit in github (https://github.com/grabs/moodle/tree/MDL-40519_master_with_mform) where it shows a possible way to do that. A side effect of this is, that the pictures are shown in a correct way. So if my way is accepted by the HQ so your issue is solved too.

              Best regards
              Andreas

              Show
              grabs Andreas Grabs added a comment - Hi Heiko, I am working on moving the nonrespondents list into a mform ( MDL-40519 ). Therefore I've made an example commit in github ( https://github.com/grabs/moodle/tree/MDL-40519_master_with_mform ) where it shows a possible way to do that. A side effect of this is, that the pictures are shown in a correct way. So if my way is accepted by the HQ so your issue is solved too. Best regards Andreas
              Hide
              schach Heiko Schach added a comment -

              Hi Andreas.
              Thanks for letting me know. Will your work have an effect on "Show responses" as well?

              Here's my fix anyway so you can decide if you want to use it or not.
              All this fix does is to define the 5px CSS-rule unambiguously for the bar images on the "Analysis" tab.

              Show
              schach Heiko Schach added a comment - Hi Andreas. Thanks for letting me know. Will your work have an effect on "Show responses" as well? Here's my fix anyway so you can decide if you want to use it or not. All this fix does is to define the 5px CSS-rule unambiguously for the bar images on the "Analysis" tab.
              Hide
              nebgor Aparup Banerjee added a comment -

              This looks good to me. I'll ping Marina Glancy as she's reviewing the work from Andreas too.
              thanks for all your feedback !

              Show
              nebgor Aparup Banerjee added a comment - This looks good to me. I'll ping Marina Glancy as she's reviewing the work from Andreas too. thanks for all your feedback !
              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
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              poltawski Dan Poltawski added a comment -

              Hello Heiko,

              Thanks very much for this fix - it looks good to me.

              Please can you provide testing instructions to ensure we can verify this change fixes the issue described and doesn't cause other regressions?

              Show
              poltawski Dan Poltawski added a comment - Hello Heiko, Thanks very much for this fix - it looks good to me. Please can you provide testing instructions to ensure we can verify this change fixes the issue described and doesn't cause other regressions?
              Hide
              schach Heiko Schach added a comment -

              Hi Dan,
              Added some testing instructions to the description.

              Show
              schach Heiko Schach added a comment - Hi Dan, Added some testing instructions to the description.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks integrated to 27, 26 and master

              Show
              poltawski Dan Poltawski added a comment - Thanks integrated to 27, 26 and master
              Hide
              fred Frédéric Massart added a comment -

              Passing, but that required a bit of digging to set up the feedback properly.

              Show
              fred Frédéric Massart added a comment - Passing, but that required a bit of digging to set up the feedback properly.
              Hide
              poltawski Dan Poltawski added a comment -

              This change is now part of Moodle! Thanks for your contribution!

              Before software can be reusable it first has to be usable.
              --Ralph Johnson

              Show
              poltawski Dan Poltawski added a comment - This change is now part of Moodle! Thanks for your contribution! Before software can be reusable it first has to be usable. --Ralph Johnson

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/Jul/14