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

      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

          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: