Moodle
  1. Moodle
  2. MDL-32380

Formal white is missing a style for form-warning class.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      Go to your local config.php.
      Write, for instance, a wrong value for a $CFG property.
      Let's say: $CFG->debug = 12345;
      Save.
      Browse your moodle to Site administration -> Development -> Debugging and look at the "Debug messages" item.
      You are supposed to see: Invalid current value: 12345 in black over a white background.
      Applying the patch the same text should be better visible.

      Show
      Go to your local config.php. Write, for instance, a wrong value for a $CFG property. Let's say: $CFG->debug = 12345; Save. Browse your moodle to Site administration -> Development -> Debugging and look at the "Debug messages" item. You are supposed to see: Invalid current value: 12345 in black over a white background. Applying the patch the same text should be better visible.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32380_master
    • Rank:
      39236

      Description

      I still had $CFG->debug = 38911 in my config.php. Going to Settings > Site Administration > Development I found the un-styled form-warning div that I had never seen.
      This issue is for the style for that div.

        Activity

        Hide
        Michael de Raadt added a comment -

        Thanks for reporting this, Daniele, and thanks for putting up a patch.

        Perhaps this would be better if the chance was added to the base theme.

        I'm also curious why you didn't see the error, even if it wasn't styled. Was it there but not prominent?

        Show
        Michael de Raadt added a comment - Thanks for reporting this, Daniele, and thanks for putting up a patch. Perhaps this would be better if the chance was added to the base theme. I'm also curious why you didn't see the error, even if it wasn't styled. Was it there but not prominent?
        Hide
        Daniele Cordella added a comment - - edited

        Thanks Michael.
        You may want to triage also MDL-32355 and MDL-32356 on the same component.

        About adding this issue to base theme... I 100% agree. I also asked for the same for MDL-29367 and MDL-31712 fix but...

        About your curiosity: well the change to $CFG->debug value is not so old and I still had never called the developer settings page!

        Let me close with a question: Am I supposed to look for a peer reviewer or the process goes by itself?

        Show
        Daniele Cordella added a comment - - edited Thanks Michael. You may want to triage also MDL-32355 and MDL-32356 on the same component. About adding this issue to base theme... I 100% agree. I also asked for the same for MDL-29367 and MDL-31712 fix but... About your curiosity: well the change to $CFG->debug value is not so old and I still had never called the developer settings page! Let me close with a question: Am I supposed to look for a peer reviewer or the process goes by itself?
        Hide
        Dan Poltawski added a comment -

        Hi Mary,

        A final theme one I wonder if you can look at, again if not please shout at me and i'll find someone else
        thanks
        dan

        Show
        Dan Poltawski added a comment - Hi Mary, A final theme one I wonder if you can look at, again if not please shout at me and i'll find someone else thanks dan
        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
        Sam Hemelryk added a comment -

        Hi Daniele,

        I was just having a quick look at this.
        A couple of things just to get your feedback on:

        Blink is an unreliable text-decoration attribute. It's not supported in IE, Chrome, or Safari. This leads the text to just be static on those browsers, pretty much just leading to a different display for those users who are using Firefox. Personally I'm not a fan of blink, blinking text in a page annoys me and I find it hard to read. I think perhaps it may be worth trying to find another way to make that text promininent. On that note I also wondered about the colour, it is pretty washed out and blends into things (an observation I made in Chrome which I was using for testing at the time where it doesn't blink).
        Anyway food for thought and I'll leave it up to you.
        Perhaps worth commenting here so that when an integrator picks it up there is a reply to this message.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Daniele, I was just having a quick look at this. A couple of things just to get your feedback on: Blink is an unreliable text-decoration attribute. It's not supported in IE, Chrome, or Safari. This leads the text to just be static on those browsers, pretty much just leading to a different display for those users who are using Firefox. Personally I'm not a fan of blink, blinking text in a page annoys me and I find it hard to read. I think perhaps it may be worth trying to find another way to make that text promininent. On that note I also wondered about the colour, it is pretty washed out and blends into things (an observation I made in Chrome which I was using for testing at the time where it doesn't blink). Anyway food for thought and I'll leave it up to you. Perhaps worth commenting here so that when an integrator picks it up there is a reply to this message. Cheers Sam
        Hide
        Daniele Cordella added a comment -

        Thanks Sam. I agree with your comments.
        I will change the commit according to your suggestions.
        Thanks again.

        Show
        Daniele Cordella added a comment - Thanks Sam. I agree with your comments. I will change the commit according to your suggestions. Thanks again.
        Hide
        Daniele Cordella added a comment -

        Sam, in the meanwhile, can I ask you to give some time to MDL-26760, please? In Italy we usually say "it already walks by itself" to let understand its age. Thanks in advance.

        Show
        Daniele Cordella added a comment - Sam, in the meanwhile, can I ask you to give some time to MDL-26760 , please? In Italy we usually say "it already walks by itself" to let understand its age. Thanks in advance.
        Hide
        Daniele Cordella added a comment -

        Ok. I changed it according to Sam's suggestions.

        Show
        Daniele Cordella added a comment - Ok. I changed it according to Sam's suggestions.
        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
        Sam Hemelryk added a comment -

        Thanks Daniele. This has been integrated now.
        Unfortunately in respect to MDL-26760 I don't know when I will have time sorry.
        My role at HQ has changed greatly in the past year and I now spend nearly all of my time reviewing code written by others and not working on code myself.
        That issue will of course remain on my todo list, however as its not high priority it may not get added to any sprints any time soon.
        Sorry about that.

        Sam

        Show
        Sam Hemelryk added a comment - Thanks Daniele. This has been integrated now. Unfortunately in respect to MDL-26760 I don't know when I will have time sorry. My role at HQ has changed greatly in the past year and I now spend nearly all of my time reviewing code written by others and not working on code myself. That issue will of course remain on my todo list, however as its not high priority it may not get added to any sprints any time soon. Sorry about that. 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
        Dan Poltawski added a comment -

        Congratulations!

        Your work has made into the latest Moodle release!

        You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

        Show
        Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

          People

          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: