Moodle
  1. Moodle
  2. MDL-34724

On the Messaging settings the label of the checkboxes at the bottom is not in a <label>

    Details

    • Rank:
      43191

      Description

      If you go to your profile and then to your messaging setting there are two tickboxes at the bottom, one for "Temporarily disable notifications" and one for "Prevent non-contacts from messaging me" but the label text is not in a label.

      This makes them less accessible and harder to theme.

        Activity

        Show
        Lalit Khattar added a comment - Bug fixed. Refer: https://github.com/D-Ne0/moodle/commit/cb8e5c19ca3838d72447b2056d690dcc8d24adb1
        Hide
        Andrew Davis added a comment -

        Hi. I've pulled that commit into my github and produced versions for master, 2.4 and 2.3. I'm also adding testing instructions.

        Show
        Andrew Davis added a comment - Hi. I've pulled that commit into my github and produced versions for master, 2.4 and 2.3. I'm also adding testing instructions.
        Hide
        Frédéric Massart added a comment -

        Hi Andrew,

        it looks good to me. As you're editing the line perhaps you could have it fit below 132 chars. Also the styles are not really nice, we could use a padding (or worse case scenario: a space) between the checkbox and the label.

        Feel free to adapt or push forward.

        Cheers!
        Fred

        Show
        Frédéric Massart added a comment - Hi Andrew, it looks good to me. As you're editing the line perhaps you could have it fit below 132 chars. Also the styles are not really nice, we could use a padding (or worse case scenario: a space) between the checkbox and the label. Feel free to adapt or push forward. Cheers! Fred
        Hide
        Andrew Davis added a comment -

        Hi Fred. I've added a second commit that reshuffles the code a bit for readability. I've also added some css to add some space between the label and the checkbox. I'm not sure if there is a better way to do this. I've added padding on the left and right so it will work with both LTR and RTL languages.

        For the moment the new commit is only in master. I'll cherry pick it to 2.4 and 2.3 after peer review.

        Show
        Andrew Davis added a comment - Hi Fred. I've added a second commit that reshuffles the code a bit for readability. I've also added some css to add some space between the label and the checkbox. I'm not sure if there is a better way to do this. I've added padding on the left and right so it will work with both LTR and RTL languages. For the moment the new commit is only in master. I'll cherry pick it to 2.4 and 2.3 after peer review.
        Hide
        Frédéric Massart added a comment -

        Thanks for fixing that Andrew.

        Working on those icons for 2.4 made me mute into a horrible CSS reviewer I'm afraid. The good thing is that we don't have any guide lines, so you can just ignore me if you want to !

        1. I don't think adding a padding to both sides of an element to handle the RTL is a good idea in each case. In this example, you're adding an extra padding between the help icon and the label;
        2. I think a padding of 3px (or .4em, again no guideline) is enough;
        3. As much as you can, avoid to use IDs in your CSS rules. The main reason being that it's not recommended (ahah), and also it makes it much more difficult for a themers to overwrite the rules. An ID weighs much more than class names. Although I don't think you have another choice in this case;
        4. I'd set the padding on the checkbox, but I have no argument to defend that lol;
        5. I'd add vertical-align: text-bottom to the checkbox, looks a bit more aesthetic to me.

        Many thanks!

        Fred

        Show
        Frédéric Massart added a comment - Thanks for fixing that Andrew. Working on those icons for 2.4 made me mute into a horrible CSS reviewer I'm afraid. The good thing is that we don't have any guide lines, so you can just ignore me if you want to ! I don't think adding a padding to both sides of an element to handle the RTL is a good idea in each case. In this example, you're adding an extra padding between the help icon and the label; I think a padding of 3px (or .4em, again no guideline) is enough; As much as you can, avoid to use IDs in your CSS rules. The main reason being that it's not recommended (ahah), and also it makes it much more difficult for a themers to overwrite the rules. An ID weighs much more than class names. Although I don't think you have another choice in this case; I'd set the padding on the checkbox, but I have no argument to defend that lol; I'd add vertical-align: text-bottom to the checkbox, looks a bit more aesthetic to me. Many thanks! Fred
        Hide
        Mary Evans added a comment -

        The real problem with Moodle CSS is that it is a mess and much of it is caused by not putting the classes to good use. If the class label is uses then .label should have a set of pre-defined CSS rules attached to it so that every time it is added to an object, be it a button, or a heading, or whatever, all those .label objects should look the same. Whereas the way you are doing it, by styling each element differently, is defeating the object of Object Oriented CSS which is what we are currently striving for.

        Show
        Mary Evans added a comment - The real problem with Moodle CSS is that it is a mess and much of it is caused by not putting the classes to good use. If the class label is uses then .label should have a set of pre-defined CSS rules attached to it so that every time it is added to an object, be it a button, or a heading, or whatever, all those .label objects should look the same. Whereas the way you are doing it, by styling each element differently, is defeating the object of Object Oriented CSS which is what we are currently striving for.
        Hide
        Mary Evans added a comment -

        David added you as a watcher. Looks like another label you missed!

        Show
        Mary Evans added a comment - David added you as a watcher. Looks like another label you missed!
        Hide
        David Scotson added a comment -

        Hi Mary,

        this is a label tag, not a .label class so it's okay for that other bug (also, I actually reported this missing label (tag) in the first place )

        As far as CSS for checkboxes, my long term goal is to try and get everyone to shift to doing it the Bootstrap way, which is to wrap the checkbox inside the label tag with an extra "checkbox" class. Then the global styles would kick in and make everything line up nicely (which is suprisingly hard to do with checkboxes and radio buttons across multiple browsers, that's why I'm so keen to use a 3rd party source that's done all that boring stuff for us)

        Search this page for "Checkboxes and Radios" to see what they do, and what it looks like:
        http://twitter.github.com/bootstrap/base-css.html

        Note, that in the past doing it that way was less accessible, and people recommended using the seperate label and using "for" to conect the label but that's no longer the case.

        Show
        David Scotson added a comment - Hi Mary, this is a label tag, not a .label class so it's okay for that other bug (also, I actually reported this missing label (tag) in the first place ) As far as CSS for checkboxes, my long term goal is to try and get everyone to shift to doing it the Bootstrap way, which is to wrap the checkbox inside the label tag with an extra "checkbox" class. Then the global styles would kick in and make everything line up nicely (which is suprisingly hard to do with checkboxes and radio buttons across multiple browsers, that's why I'm so keen to use a 3rd party source that's done all that boring stuff for us) Search this page for "Checkboxes and Radios" to see what they do, and what it looks like: http://twitter.github.com/bootstrap/base-css.html Note, that in the past doing it that way was less accessible, and people recommended using the seperate label and using "for" to conect the label but that's no longer the case.
        Hide
        Andrew Davis added a comment -

        David, shifting many of Moodle's checkboxes over to the Bootstrap way should theoretically just be a matter of altering html_writer::checkbox() in lib/outputcomponents.php. That's what the messaging config screen is using and presumably quite a few other places. The messaging config screen calls html_writer::checkbox() and that function produces both the checkbox and any associated label.

        Show
        Andrew Davis added a comment - David, shifting many of Moodle's checkboxes over to the Bootstrap way should theoretically just be a matter of altering html_writer::checkbox() in lib/outputcomponents.php. That's what the messaging config screen is using and presumably quite a few other places. The messaging config screen calls html_writer::checkbox() and that function produces both the checkbox and any associated label.
        Hide
        Andrew Davis added a comment -

        I'm actually thinking that I may put this up for integration minus the css. I'm in agreement that we shouldn't be styling common elements individually. Some sort of larger scale solution is in order.

        Show
        Andrew Davis added a comment - I'm actually thinking that I may put this up for integration minus the css. I'm in agreement that we shouldn't be styling common elements individually. Some sort of larger scale solution is in order.
        Hide
        David Scotson added a comment -

        Hi Andrew,

        I think there's a few items like that where small simple changes to core could have a big impact, the Quizzes, User facing forms and Admin forms all make good use of standard function calls so with 20% of the effort you could convert 80% of Moodle's form inputs to modern Bootstrap-like conventions.

        I intend to submit the core changes for integration in the 2.6 dev cycle, hopefully recruiting some help to fix at least some of the many other corner cases and hand-coded areas to match too (though that might take till 2.7 to complete, depending on how many people contribute).

        Show
        David Scotson added a comment - Hi Andrew, I think there's a few items like that where small simple changes to core could have a big impact, the Quizzes, User facing forms and Admin forms all make good use of standard function calls so with 20% of the effort you could convert 80% of Moodle's form inputs to modern Bootstrap-like conventions. I intend to submit the core changes for integration in the 2.6 dev cycle, hopefully recruiting some help to fix at least some of the many other corner cases and hand-coded areas to match too (though that might take till 2.7 to complete, depending on how many people contribute).
        Hide
        Aparup Banerjee added a comment -

        cool, i've integrated that into 23, 24 and master.

        ps: MDL-38016 is up for peer-review, i'm sure much review is welcome there

        Show
        Aparup Banerjee added a comment - cool, i've integrated that into 23, 24 and master. ps: MDL-38016 is up for peer-review, i'm sure much review is welcome there
        Hide
        Ankit Agarwal added a comment -

        Works as described. Checkboxs now have labels.
        Thanks

        Show
        Ankit Agarwal added a comment - Works as described. Checkboxs now have labels. Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

        Thanks!

        PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

        Show
        Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: