Moodle
  1. Moodle
  2. MDL-46063

User pictures in Feedback module squeezed to 5px

    Details

      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

          Issue Links

            Activity

            Hide
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            CiBoT added a comment -

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

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            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
            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
            Heiko Schach added a comment -

            Hi Dan,
            Added some testing instructions to the description.

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

            Thanks integrated to 27, 26 and master

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

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

            Show
            Frédéric Massart added a comment - Passing, but that required a bit of digging to set up the feedback properly.
            Hide
            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
            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: