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

Multichoice answers editor fills the whole region width

    Details

    • Testing Instructions:
      Hide

      This needs to be tested with all browsers and all themes, or at least a reasonable sample there-of.

      1. Purge all caches
      2. Remove browser session
      3. Edit/create a multiple choices question (do not click on the 'save changes' button)
      4. Set browser window to fullsize screen
      5. Make sure choice answer label and editor appears on the same line
      6. Set broswer window to use 1024px width
      7. Make sure choice answer label appear on the top of text editor. Also make sure the label has some padding on the left.
      8. Repeat the test for other browsers and other themes.
      Show
      This needs to be tested with all browsers and all themes, or at least a reasonable sample there-of. Purge all caches Remove browser session Edit/create a multiple choices question (do not click on the 'save changes' button) Set browser window to fullsize screen Make sure choice answer label and editor appears on the same line Set broswer window to use 1024px width Make sure choice answer label appear on the top of text editor. Also make sure the label has some padding on the left. Repeat the test for other browsers and other themes.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      According to what has been reported here:

      In the latest weekly master it looks good.

      (Edited: Removing 1st screenshot as was unrelated)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dmonllao David Monllaó added a comment -

            Marking as a regression of MDL-42887 but not 100% sure, at least not present in last 2.6 beta

            Show
            dmonllao David Monllaó added a comment - Marking as a regression of MDL-42887 but not 100% sure, at least not present in last 2.6 beta
            Hide
            timhunt Tim Hunt added a comment -

            Michael de Raadt I am assuming that Moodle HQ FRONTEND team will be working on this.

            Show
            timhunt Tim Hunt added a comment - Michael de Raadt I am assuming that Moodle HQ FRONTEND team will be working on this.
            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Tim.

            The FRONTEND team should work on this. They are likely to come to you for peer review. Feel free to jump in before that if you want to have a go.

            Show
            salvetore Michael de Raadt added a comment - Hi, Tim. The FRONTEND team should work on this. They are likely to come to you for peer review. Feel free to jump in before that if you want to have a go.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi David,

            I'm unable to reproduce this issue for 2.6 and master.

            Could you confirm on this?

            Thanks.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi David, I'm unable to reproduce this issue for 2.6 and master. Could you confirm on this? Thanks.
            Hide
            dmonllao David Monllaó added a comment -

            Attaching a screenshot of current master (b58bc15af5fff22adca0285e96689614ba353931) using clean theme + adding a multichoice question

            IMO this looks wrong

            Show
            dmonllao David Monllaó added a comment - Attaching a screenshot of current master (b58bc15af5fff22adca0285e96689614ba353931) using clean theme + adding a multichoice question IMO this looks wrong
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Thanks David for the screenshots.

            Adding patch to fix the issue and assigning Damyon as peer-reviewer.

            Show
            rwijaya Rossiani Wijaya added a comment - Thanks David for the screenshots. Adding patch to fix the issue and assigning Damyon as peer-reviewer.
            Hide
            timhunt Tim Hunt added a comment -

            If you change question/type/multichoice/styles.css then the testing instructions have to say "Please test all themes."

            Also if you look at the screen-grab, then the issue is not the absence of 6px padding. The problem is that the editor is wrapped onto the following line. With The screen as wide as in that screen-grab, the editor should be beside the label, like all the other form fields.

            Show
            timhunt Tim Hunt added a comment - If you change question/type/multichoice/styles.css then the testing instructions have to say "Please test all themes." Also if you look at the screen-grab, then the issue is not the absence of 6px padding. The problem is that the editor is wrapped onto the following line. With The screen as wide as in that screen-grab, the editor should be beside the label, like all the other form fields.
            Hide
            timhunt Tim Hunt added a comment -

            So, actually, this is not ready for peer review. It still needs more work. Fixing workflow status.

            Show
            timhunt Tim Hunt added a comment - So, actually, this is not ready for peer review. It still needs more work. Fixing workflow status.
            Hide
            rwijaya Rossiani Wijaya added a comment - - edited

            Hi Tim,

            Thank you for the feedback.

            The patch will fix the following:

            1. When window size is >= 1200px, the texteditor will be displayed inline with label.
            2. When window size is < 1200px, the texteditor will be displayed under the label and the label will be indented by 6px (to improve the label appearance).

            When you have a chance could you please peer-review this again?

            PS: I've updated the testing instruction and will backport the patch to 2.6 once it passed the peer-review stage.

            Show
            rwijaya Rossiani Wijaya added a comment - - edited Hi Tim, Thank you for the feedback. The patch will fix the following: When window size is >= 1200px, the texteditor will be displayed inline with label. When window size is < 1200px, the texteditor will be displayed under the label and the label will be indented by 6px (to improve the label appearance). When you have a chance could you please peer-review this again? PS: I've updated the testing instruction and will backport the patch to 2.6 once it passed the peer-review stage.
            Hide
            timhunt Tim Hunt added a comment -

            OK, good. That is the behaviour we want. The code looks OK. I am not sure why these CSS changes fix the bug, but I assume you do, and I just have not thought about it hard enough.

            Please back-port and submit for integration.

            Show
            timhunt Tim Hunt added a comment - OK, good. That is the behaviour we want. The code looks OK. I am not sure why these CSS changes fix the bug, but I assume you do, and I just have not thought about it hard enough. Please back-port and submit for integration.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Tim,

            Thank you for reviewing.

            The margin value increase the width for the field and if the total width greater than the set width, it will force the next element to the new line.

            I have backported the patch to 2.6.

            Submitting for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Tim, Thank you for reviewing. The margin value increase the width for the field and if the total width greater than the set width, it will force the next element to the new line. I have backported the patch to 2.6. Submitting for integration review.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Rosie. This has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Rosie. This has been integrated now.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Tested against:

            • Safari
            • Firefox
            • Chrome
            • IE

            On:

            • Clean
            • Standard
            • Brick
            Show
            dobedobedoh Andrew Nicols added a comment - Tested against: Safari Firefox Chrome IE On: Clean Standard Brick
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your contributions, this change is now upstream!

            “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

            Show
            poltawski Dan Poltawski added a comment - Thanks for your contributions, this change is now upstream! “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/Jan/14