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
    • Rank:
      18601

      Description

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

        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: