Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: HTML and CSS
    • Labels:
    • Testing Instructions:
      Hide

      Go to login page while using Simple theme.

      Attemp to log in with an incorrect password.

      The error message should appear in the standard error shade of red, but it should not be in a red/pink alert box.

      Show
      Go to login page while using Simple theme. Attemp to log in with an incorrect password. The error message should appear in the standard error shade of red, but it should not be in a red/pink alert box.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-39196-master

      Description

      The error messages on login are styled as an alert (a colored box), but is currently wrapped in a span, which is display: inline by default. This causes visual problems when the text gets long enough (or the screen narrow enough) to cause line wrapping.

      One option is to set display: block on the span but then you need to constrain it's width.

      The other option is to remove the alert styling and just have red text.

      For the short term I'd probably favour the latter.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            agroshek Amy Groshek added a comment -

            David Scotson, what's your preferred the long-term solution? Might be worth capturing that as well, even if in a separate task with a longer timeline.

            Show
            agroshek Amy Groshek added a comment - David Scotson , what's your preferred the long-term solution? Might be worth capturing that as well, even if in a separate task with a longer timeline.
            Hide
            bawjaws David Scotson added a comment -

            If the login page used a grid (either two columns taking up half the screen or a single column offset into the middle depending on settings), then that would constrain the alert's width and would probably work best particularly if the rest of the login was shuffled so that it was all in harmony.

            There's a bunch of other admin alerts in admin.less were we currently set the size to 60% and set a min width, basically manually simulating a collapsing grid, which is what I meant by option one, but it adds a lot of code compared with just making the text red, and doesn't look as good when the alert block is crowded like it is on the login page.

            Show
            bawjaws David Scotson added a comment - If the login page used a grid (either two columns taking up half the screen or a single column offset into the middle depending on settings), then that would constrain the alert's width and would probably work best particularly if the rest of the login was shuffled so that it was all in harmony. There's a bunch of other admin alerts in admin.less were we currently set the size to 60% and set a min width, basically manually simulating a collapsing grid, which is what I meant by option one, but it adds a lot of code compared with just making the text red, and doesn't look as good when the alert block is crowded like it is on the login page.
            Hide
            phalacee Jason Fowler added a comment -

            The alert styling (icon) was added for accessibility purposes.

            Show
            phalacee Jason Fowler added a comment - The alert styling (icon) was added for accessibility purposes.
            Hide
            bawjaws David Scotson added a comment -

            @ Jason, by "alert" we don't mean the ! icon. An "alert" in Bootstrap is a colored div with the class "alert".

            http://twitter.github.io/bootstrap/components.html#alerts

            Show
            bawjaws David Scotson added a comment - @ Jason, by "alert" we don't mean the ! icon. An "alert" in Bootstrap is a colored div with the class "alert". http://twitter.github.io/bootstrap/components.html#alerts
            Hide
            bawjaws David Scotson added a comment - - edited

            I've just noticed this is set to "Must fix for 2.5". It only really shows up if a) you have a login error, b) you have long text in the error, and the screen is narrow. Can I just remove that from stuff or is there someone in charge of these decisions I need to convince?

            edit: not that it's a big fix, just not a stop ship priority imho

            Show
            bawjaws David Scotson added a comment - - edited I've just noticed this is set to "Must fix for 2.5". It only really shows up if a) you have a login error, b) you have long text in the error, and the screen is narrow. Can I just remove that from stuff or is there someone in charge of these decisions I need to convince? edit: not that it's a big fix, just not a stop ship priority imho
            Hide
            agroshek Amy Groshek added a comment -

            I think you can just remove it, David. I was wondering why it was set to that when I triaged. I changed to 2.6 for now.

            Show
            agroshek Amy Groshek added a comment - I think you can just remove it, David. I was wondering why it was set to that when I triaged. I changed to 2.6 for now.
            Hide
            bawjaws David Scotson added a comment -

            I've done something weird with the commits, I'll try and fix that, but the actual change is pretty minor.

            Show
            bawjaws David Scotson added a comment - I've done something weird with the commits, I'll try and fix that, but the actual change is pretty minor.
            Hide
            lazydaisy Mary Evans added a comment -

            What's all the fuss about with this?

            Show
            lazydaisy Mary Evans added a comment - What's all the fuss about with this?
            Hide
            lazydaisy Mary Evans added a comment -

            I've set it for integration review.

            Show
            lazydaisy Mary Evans added a comment - I've set it for integration review.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks David, I've integrated this fix now.

            There was a bit of weirdness going on with the commits, I fixed that during integration. There's now just a single commit with a meaningful message and yourself set as author.

            As for this change itself, indeed not exactly what you'd call a blocker to release however your fix is fine and the end outcome looks good so I was happy to put it in.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks David, I've integrated this fix now. There was a bit of weirdness going on with the commits, I fixed that during integration. There's now just a single commit with a meaningful message and yourself set as author. As for this change itself, indeed not exactly what you'd call a blocker to release however your fix is fine and the end outcome looks good so I was happy to put it in. Many thanks Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Tested during integration review and passed

            Show
            samhemelryk Sam Hemelryk added a comment - Tested during integration review and passed
            Hide
            lazydaisy Mary Evans added a comment -

            Thanks Sam

            Show
            lazydaisy Mary Evans added a comment - Thanks Sam
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I feel myself really alone tonight! So was time to push your fixes upstream!

            "Lest we forget. We will remember them."

            Thanks and ciao!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

              People

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

                Dates

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