Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-34724

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

    Details

    • Testing Instructions:
      Hide

      Go to the messaging settings page under my profile settings.

      Check that the text next to the checkboxes in the general settings section is in labels.

      Show
      Go to the messaging settings page under my profile settings. Check that the text next to the checkboxes in the general settings section is in labels.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-35582_message_labels

      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.

        Gliffy Diagrams

          Activity

          Show
          d-ne0 Lalit Khattar added a comment - Bug fixed. Refer: https://github.com/D-Ne0/moodle/commit/cb8e5c19ca3838d72447b2056d690dcc8d24adb1
          Hide
          andyjdavis 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
          andyjdavis 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
          fred 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
          fred 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
          andyjdavis 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
          andyjdavis 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
          fred 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
          fred 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
          lazydaisy 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
          lazydaisy 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
          lazydaisy Mary Evans added a comment -

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

          Show
          lazydaisy Mary Evans added a comment - David added you as a watcher. Looks like another label you missed!
          Hide
          bawjaws 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
          bawjaws 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
          andyjdavis 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
          andyjdavis 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
          andyjdavis 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
          andyjdavis 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
          bawjaws 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
          bawjaws 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
          nebgor 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
          nebgor 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_frenz Ankit Agarwal added a comment -

          Works as described. Checkboxs now have labels.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Works as described. Checkboxs now have labels. Thanks
          Hide
          stronk7 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
          stronk7 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:
                Fix Release Date:
                18/Mar/13