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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            salvetore 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
            salvetore 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
            salvetore Michael de Raadt added a comment -

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

            Show
            salvetore Michael de Raadt added a comment - And could you update the title to reflect where the problem is.
            Hide
            nebgor 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
            nebgor 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
            nebgor 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
            nebgor 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
            rajeshtaneja Rajesh Taneja added a comment -

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

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Aparup, I am adding Helen here to provide some inputs on this
            Hide
            tsala 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
            tsala 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
            nebgor 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
            nebgor 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
            rajeshtaneja Rajesh Taneja added a comment -

            Is it fine if two check boxes are aligned vertically?

            Show
            rajeshtaneja Rajesh Taneja added a comment - Is it fine if two check boxes are aligned vertically?
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            salvetore 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
            salvetore 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
            rajeshtaneja 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
            rajeshtaneja 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
            tsala 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
            tsala 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
            stronk7 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
            stronk7 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
            Hide
            stronk7 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
            stronk7 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            rajeshtaneja Rajesh Taneja added a comment -

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

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for the feedback Sam My +1 for updating existing branches with your feedback before this gets integrated
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            stronk7 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
            stronk7 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
            samhemelryk 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
            samhemelryk 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
            andyjdavis 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
            andyjdavis 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            Ciao

            Show
            stronk7 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:
                  Fix Release Date:
                  10/Oct/11