Moodle
  1. Moodle
  2. MDL-29079

'Include user completion information' setting has two check boxes instead of one

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide
      • Log in as an Admin
      • Browse to Settings > Courses > Backups > General backup defaults
      • Check that there is a reasonable space between the two checkboxes
      • Change your language to a right to left language like HE (?lang=he)
      • Check that the spacing is still reasonable.
      Show
      Log in as an Admin Browse to Settings > Courses > Backups > General backup defaults Check that there is a reasonable space between the two checkboxes Change your language to a right to left language like HE (?lang=he) Check that the spacing is still reasonable.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-29079

      Description

      'Include user completion information' setting has two check boxes instead of one it seems.

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          Could you add instructions on how to get to this form?

          (Thanks for adding the arrows, by the way )

          Show
          Michael de Raadt added a comment - Could you add instructions on how to get to this form? (Thanks for adding the arrows, by the way )
          Hide
          Michael de Raadt added a comment -

          And could you update the title to reflect where the problem is.

          Show
          Michael de Raadt added a comment - And could you update the title to reflect where the problem is.
          Hide
          Aparup Banerjee added a comment - - edited

          easy done, search for the setting called 'Include user completion information' setting.. in the administration block.. or just search for 'completion' as can be seen left of screenshot .

          Show
          Aparup Banerjee added a comment - - edited easy done, search for the setting called 'Include user completion information' setting.. in the administration block.. or just search for 'completion' as can be seen left of screenshot .
          Hide
          Aparup Banerjee added a comment -

          Raj, i think I've seen some places where a select is used to select different types of options. But yes definitely 'locked' needs some rewording.

          Show
          Aparup Banerjee added a comment - Raj, i think I've seen some places where a select is used to select different types of options. But yes definitely 'locked' needs some rewording.
          Hide
          Rajesh Taneja added a comment -

          Thanks Aparup, I am adding Helen here to provide some inputs on this

          Show
          Rajesh Taneja added a comment - Thanks Aparup, I am adding Helen here to provide some inputs on this
          Hide
          Helen Foster added a comment -

          It seems that all general backup defaults have a locked checkbox which is easy to understand when you backup a course because of the little padlock icon and mouseover tooltip "This setting has been locked by the default backup settings" (see attached screenshot).

          Elsewhere in the site admin settings the word 'Force' is used, rather than 'locked', however I think 'locked' conveys the meaning better.

          Apu's screenshot shows the locked checkbox very close to the other checkbox, however when I checked demo.moodle.net (using the leatherbound theme and Firefox) I found the checkboxes a reasonable distance apart (see attached screenshot).

          I can imagine it being a bit confusing if the locked checkbox appears in a search result, however I think that all the locked checkboxes on the general backup defaults page look fine.

          Thus I suggest we leave the setting wording as 'locked'.

          Show
          Helen Foster added a comment - It seems that all general backup defaults have a locked checkbox which is easy to understand when you backup a course because of the little padlock icon and mouseover tooltip "This setting has been locked by the default backup settings" (see attached screenshot). Elsewhere in the site admin settings the word 'Force' is used, rather than 'locked', however I think 'locked' conveys the meaning better. Apu's screenshot shows the locked checkbox very close to the other checkbox, however when I checked demo.moodle.net (using the leatherbound theme and Firefox) I found the checkboxes a reasonable distance apart (see attached screenshot). I can imagine it being a bit confusing if the locked checkbox appears in a search result, however I think that all the locked checkboxes on the general backup defaults page look fine. Thus I suggest we leave the setting wording as 'locked'.
          Hide
          Aparup Banerjee added a comment -

          Yes Helen, it appearing as a search result is exactly how it appeared confusing to me.
          (ps: Raj, my screen shot was using standard theme and firefox)

          Show
          Aparup Banerjee added a comment - Yes Helen, it appearing as a search result is exactly how it appeared confusing to me. (ps: Raj, my screen shot was using standard theme and firefox)
          Hide
          Rajesh Taneja added a comment -

          Is it fine if two check boxes are aligned vertically?

          Show
          Rajesh Taneja added a comment - Is it fine if two check boxes are aligned vertically?
          Hide
          Rajesh Taneja added a comment -

          Changing orientation will probably confuse users and as Helen suggested wordings are fine, so I think adding some space makes it a bit clearer.

          Show
          Rajesh Taneja added a comment - Changing orientation will probably confuse users and as Helen suggested wordings are fine, so I think adding some space makes it a bit clearer.
          Hide
          Michael de Raadt added a comment -

          The code changes seem to work well and is a good generic solution that allows the visual aspects of the lock option checkboxes to be controlled by styles.

          I think it would be better to separate the two checkboxes vertically. At the moment it looks like the default information applies to the locked checkbox instead of the main setting checkbox. It could even appear after the description. Unfortunately, to achieve this would require changes to format_admin_setting() and I'm not sure what other functionality that would affect.

          Show
          Michael de Raadt added a comment - The code changes seem to work well and is a good generic solution that allows the visual aspects of the lock option checkboxes to be controlled by styles. I think it would be better to separate the two checkboxes vertically. At the moment it looks like the default information applies to the locked checkbox instead of the main setting checkbox. It could even appear after the description. Unfortunately, to achieve this would require changes to format_admin_setting() and I'm not sure what other functionality that would affect.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Michael,
          I thought of vertically aligning the check boxes, but that might lead to regression in some themes. Will push this for integration, to get some feedback from integrators and see if it should be vertically aligned or the current solution is enough to resolve this issue

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Michael, I thought of vertically aligning the check boxes, but that might lead to regression in some themes. Will push this for integration, to get some feedback from integrators and see if it should be vertically aligned or the current solution is enough to resolve this issue
          Hide
          Helen Foster added a comment -

          -1 for vertically aligning the check boxes as this is inconsistent with elsewhere in the admin settings UI e.g. grade category settings.

          Show
          Helen Foster added a comment - -1 for vertically aligning the check boxes as this is inconsistent with elsewhere in the admin settings UI e.g. grade category settings.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think horizontal spacing is ok. Simpler and consistent with other settings here and there also using the locking mechanism. I agree they are not visually wonderful but...

          Show
          Eloy Lafuente (stronk7) added a comment - I think horizontal spacing is ok. Simpler and consistent with other settings here and there also using the locking mechanism. I agree they are not visually wonderful but...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Side comment: When looking at this I detected that, in general, it seems that we also are printing the checkboxes and their right-to label really, really together in all the admin settings. See, for example the locked checkbox and its "locked" label (the "l" is practically on the checkbox") or any other setting in the admin tree having labels on the right. Surely some simple CSS sorcery could improve that a bit.

          For your consideration, feel free to ignore / create new issue about that. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Side comment: When looking at this I detected that, in general, it seems that we also are printing the checkboxes and their right-to label really, really together in all the admin settings. See, for example the locked checkbox and its "locked" label (the "l" is practically on the checkbox") or any other setting in the admin tree having labels on the right. Surely some simple CSS sorcery could improve that a bit. For your consideration, feel free to ignore / create new issue about that. Ciao
          Hide
          Sam Hemelryk added a comment -

          Copying testing instructions to comment (looks like that is what it was meant to be) and adding testing instructions:

          Thanks for the feedback Michael,
          I thought of vertically aligning the check boxes, but that might lead to regression in some themes. Will push this for integration, to get some feedback from integrators and see if it should be vertically aligned or the current solution is enough to resolve this issue

          Commented by Raj

          Show
          Sam Hemelryk added a comment - Copying testing instructions to comment (looks like that is what it was meant to be) and adding testing instructions: Thanks for the feedback Michael, I thought of vertically aligning the check boxes, but that might lead to regression in some themes. Will push this for integration, to get some feedback from integrators and see if it should be vertically aligned or the current solution is enough to resolve this issue Commented by Raj
          Hide
          Sam Hemelryk added a comment -

          Hmmm failing this for the time being as the spacing differs when using a RTL language like HE.
          Looking at the changes that have been made it gets my +1 to apply that margin spacing to both sides of the locked checkbox (beats adding an additional rtl rule).

          Alternatively we could just create a new issue and pass this issue

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hmmm failing this for the time being as the spacing differs when using a RTL language like HE. Looking at the changes that have been made it gets my +1 to apply that margin spacing to both sides of the locked checkbox (beats adding an additional rtl rule). Alternatively we could just create a new issue and pass this issue Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Sam
          My +1 for updating existing branches with your feedback before this gets integrated

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Sam My +1 for updating existing branches with your feedback before this gets integrated
          Hide
          Rajesh Taneja added a comment -

          Checked in one more commit, which will add space on both sides of locked checkbox and for rtl languages it will add similar spacing on both sides.
          Leaving it for integrators to decide

          Show
          Rajesh Taneja added a comment - Checked in one more commit, which will add space on both sides of locked checkbox and for rtl languages it will add similar spacing on both sides. Leaving it for integrators to decide
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ops, sorry, I always forget about RTL.

          Anyway, general Q, do we need always one RTL alternative for each rule? For example, the .defaultsnext (immediately above Rajesh change) also has one "right" selector, and it's missing its .rtl counterpart.

          I'm to reintegrate new Rajesh 2-rules solution. Surely he decided to go that why because allowed finer adjustment of both sides vs the unique rule alternative.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ops, sorry, I always forget about RTL. Anyway, general Q, do we need always one RTL alternative for each rule? For example, the .defaultsnext (immediately above Rajesh change) also has one "right" selector, and it's missing its .rtl counterpart. I'm to reintegrate new Rajesh 2-rules solution. Surely he decided to go that why because allowed finer adjustment of both sides vs the unique rule alternative. Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          Ideally we'd sort out our styles so that spacing (margin and/or padding) are applied equally of both sides so that rtl/ltr doesn't matter. Imagine a table for instance where you apply a padding on cells.
          Really the only exception would be lists where we would cleary want to indent in one direction only.

          The fewer .dir-rtl and .dir-ltr rules we need the better as we already have an over abundance of CSS we are serving.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, Ideally we'd sort out our styles so that spacing (margin and/or padding) are applied equally of both sides so that rtl/ltr doesn't matter. Imagine a table for instance where you apply a padding on cells. Really the only exception would be lists where we would cleary want to indent in one direction only. The fewer .dir-rtl and .dir-ltr rules we need the better as we already have an over abundance of CSS we are serving. Cheers Sam
          Hide
          Andrew Davis added a comment - - edited

          Works fine both left to right and right to left. We're probably being a bit conservative here and could have done with putting a bigger gap between the two but its fine.

          Show
          Andrew Davis added a comment - - edited Works fine both left to right and right to left. We're probably being a bit conservative here and could have done with putting a bigger gap between the two but its fine.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: