Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Trivial 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
    • Rank:
      49801

      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.

        Activity

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

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

        Show
        Jason Fowler added a comment - The alert styling (icon) was added for accessibility purposes.
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        Mary Evans added a comment -

        What's all the fuss about with this?

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

        I've set it for integration review.

        Show
        Mary Evans added a comment - I've set it for integration review.
        Hide
        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
        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
        Sam Hemelryk added a comment -

        Tested during integration review and passed

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

        Thanks Sam

        Show
        Mary Evans added a comment - Thanks Sam
        Hide
        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
        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: