Moodle
  1. Moodle
  2. MDL-27862

Theme selector strings need updating and unset button required to unset theme

    Details

    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Go to theme selector (Settings -> site administrator -> theme selector -> Select device)
      3. make sure navigation works fine and you can navigate to theme page and device selection page
      4. Check lang strings on change device and theme selection page (should match with description of bug, except button label)
      5. Change them on all devices and it should work fine
      6. remove theme on device and make sure theme can be unset on all devices (except default)
      Show
      Log in as admin Go to theme selector (Settings -> site administrator -> theme selector -> Select device) make sure navigation works fine and you can navigate to theme page and device selection page Check lang strings on change device and theme selection page (should match with description of bug, except button label) Change them on all devices and it should work fine remove theme on device and make sure theme can be unset on all devices (except default)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-27862
    • Rank:
      17511

      Description

      After changes made in MDL-25394, selecting a theme is now a two step process. First the user specifies a device, then they select a theme. This is not obvious now with the pre-existing strings.

      The following strings changes will make the process clearer.

      • First page title "Theme" -> "Select Device Type"
      • First page, table title "Theme" -> "Current theme" or "Current theme for device"
      • First page, button text "Select theme" -> "Select theme for device" (possibly too long)
      • Second page title "Themes" -> "Select theme for <device type>"
      • Second page, button text "Use theme" -> "Use theme for <device type>"

      As another improvement, the navigation menu could expand another level under "Theme Selector" with a sub-item for each device type.

      In addition to this:
      it's impossible to "un-select" themes for device types and perhaps it should be something to consider. Also, clear cache redirect to first page, should remain at the same page from where it is called.

      1. MDL-27862-DEFAULT.jpg
        26 kB
      2. MDL-27862-DEVICETHEMES.jpg
        26 kB
      3. MDL-27862-LEGACY.jpg
        18 kB
      4. MDL-27862-MOBILE.jpg
        17 kB
      5. MDL-27862-TABLET.jpg
        15 kB

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          This issue was duplicated by another user, but one good suggestion from them was about the text on the first button mentioned. Instead of "Select device type", we could say "Show themes for this device type".

          Show
          Michael de Raadt added a comment - This issue was duplicated by another user, but one good suggestion from them was about the text on the first button mentioned. Instead of "Select device type", we could say "Show themes for this device type".
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi,

          I just ended with one small patch related with this @ MDL-29552 (it fixes another little detail, not listed here).

          But when I was testing it, I discovered that it's impossible to "un-select" themes for device types and perhaps it should be something to consider. Just dropping the request here for your consideration.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, I just ended with one small patch related with this @ MDL-29552 (it fixes another little detail, not listed here). But when I was testing it, I discovered that it's impossible to "un-select" themes for device types and perhaps it should be something to consider. Just dropping the request here for your consideration. Ciao
          Hide
          Michael de Raadt added a comment -

          Bumping this as it has been duplicated again.

          Show
          Michael de Raadt added a comment - Bumping this as it has been duplicated again.
          Hide
          Michael de Raadt added a comment -

          There are some additional details in duplicate MDL-30952 that might need to be considered.

          Show
          Michael de Raadt added a comment - There are some additional details in duplicate MDL-30952 that might need to be considered.
          Hide
          Mary Evans added a comment - - edited

          Hi Michael, like I said in the duplicated issue you just closed, this is NOT a theme problem, although it is related to themes indirectly, this falls under Administration umbrella surely. Besides, this problem has become greater now in Moodle 2.2 as the option to select other device types, in a two-step fashion, you can't. It's not working as it did. I think Moodle 2.1 works OK still, but Moodle 2.2 seems to have regressed in this respect.

          Cheers
          Mary

          Show
          Mary Evans added a comment - - edited Hi Michael, like I said in the duplicated issue you just closed, this is NOT a theme problem, although it is related to themes indirectly, this falls under Administration umbrella surely. Besides, this problem has become greater now in Moodle 2.2 as the option to select other device types, in a two-step fashion, you can't. It's not working as it did. I think Moodle 2.1 works OK still, but Moodle 2.2 seems to have regressed in this respect. Cheers Mary
          Hide
          Michael de Raadt added a comment -

          Hi, Mary.

          Sorry if you though I was dumping work on you. You are correct, this isn't about themes, it's more about the admin interface.

          Thanks for assigning this to Raj. we'll handle this here at HQ.

          Michael d;

          Show
          Michael de Raadt added a comment - Hi, Mary. Sorry if you though I was dumping work on you. You are correct, this isn't about themes, it's more about the admin interface. Thanks for assigning this to Raj. we'll handle this here at HQ. Michael d;
          Hide
          Rajesh Taneja added a comment -

          Have added three commits:

          1. Strings changed on bothe theme pages
          2. Navigation added
          3. Un-select theme option added for devices (except default device)

          Just wondering, if we should have device name as lang string.

          Show
          Rajesh Taneja added a comment - Have added three commits: Strings changed on bothe theme pages Navigation added Un-select theme option added for devices (except default device) Just wondering, if we should have device name as lang string.
          Hide
          Rajesh Taneja added a comment -

          @Helen:
          Can you please check if updated strings are fine. I have changed string on button to "Change theme" and not one described in bug.
          IMO, Buttons should have small label.

          Show
          Rajesh Taneja added a comment - @Helen: Can you please check if updated strings are fine. I have changed string on button to "Change theme" and not one described in bug. IMO, Buttons should have small label.
          Hide
          Helen Foster added a comment -

          Hi Raj,

          Please could 'Select Device Type' be changed to 'Select device type' - i.e. only capitalizing the first word as per http://docs.moodle.org/dev/Coding_style#Language_strings

          Also wondering whether it is necessary to have lang strings 'Select device type' AND 'Select device'? Maybe it is (I don't know) as I notice the strings are in different lang files.

          Show
          Helen Foster added a comment - Hi Raj, Please could 'Select Device Type' be changed to 'Select device type' - i.e. only capitalizing the first word as per http://docs.moodle.org/dev/Coding_style#Language_strings Also wondering whether it is necessary to have lang strings 'Select device type' AND 'Select device'? Maybe it is (I don't know) as I notice the strings are in different lang files.
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing that Helen.
          You are right, we don't need two strings, hence one removed. Also, fixed capitalization string.

          Show
          Rajesh Taneja added a comment - Thanks for pointing that Helen. You are right, we don't need two strings, hence one removed. Also, fixed capitalization string.
          Hide
          Ankit Agarwal added a comment -

          Looks good
          +1 to integrate!
          Thanks

          Show
          Ankit Agarwal added a comment - Looks good +1 to integrate! Thanks
          Hide
          Rajesh Taneja added a comment -

          Thanks Ankit,

          I will put this up for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Ankit, I will put this up for integration review.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm,

          sorry but, or I'm reading/getting wrongly or I think we are creating some UI redundancies I'm not sure if they are a good idea.

          1) You've added one extra admin category to enclose all the "theme selection" items, perfect, although it's one more level (prone to warp).

          2) You've created one admin item for each device type, in order to replace the 2-screens setup, with a direct navigation to the picked device (1-less click). Oki.

          3) But, while the "2-screens" approach has been replaced with the direct navigation alternative in 2) for setting and changing themes, at the same time you are keeping the old "2-screens" UI just for deleting one theme from the device.

          IMO we should take rid of the "2-screens" completely and perform the deletion from each "device" page (but for "default", agree). To keep both UIs is really confusing, at least here.

          So, I'd propose to:

          1) Take rid of any link to the "2-screens" UI.
          2) Add the "Delete" option (perhaps should be called "Unset", coz we aren't really deleting anything) to the Device theme picker (but to the "default" device).
          3) Also, I've observed that, for the picked theme, it's shown inside a "black border" to mark it as the selected one. But, at the same time, the "Use theme" button continues being shown, but disabled or so. IMO we should not show it at all, it's already in use. Consider deleting it and/or changing it to one "Currently in use" message.
          4) Also, the new admin pages are named "Change XXXX theme". IMO, both the "Change" and the "theme" are really unnecessary. Why not show the device type (surely with a complete "title") and done? That would prevent some warping in the block.
          5) Finally, about code, apart of some commas not followed by whitespace, and some "//" comments not followed by whitespace (not sure if this is ruled), it looks correct.

          So I'm reopening this to think a bit more about the way to avoid the "double UI" thingy together with the rest of minor points.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, sorry but, or I'm reading/getting wrongly or I think we are creating some UI redundancies I'm not sure if they are a good idea. 1) You've added one extra admin category to enclose all the "theme selection" items, perfect, although it's one more level (prone to warp). 2) You've created one admin item for each device type, in order to replace the 2-screens setup, with a direct navigation to the picked device (1-less click). Oki. 3) But, while the "2-screens" approach has been replaced with the direct navigation alternative in 2) for setting and changing themes, at the same time you are keeping the old "2-screens" UI just for deleting one theme from the device. IMO we should take rid of the "2-screens" completely and perform the deletion from each "device" page (but for "default", agree). To keep both UIs is really confusing, at least here. So, I'd propose to: 1) Take rid of any link to the "2-screens" UI. 2) Add the "Delete" option (perhaps should be called "Unset", coz we aren't really deleting anything) to the Device theme picker (but to the "default" device). 3) Also, I've observed that, for the picked theme, it's shown inside a "black border" to mark it as the selected one. But, at the same time, the "Use theme" button continues being shown, but disabled or so. IMO we should not show it at all, it's already in use. Consider deleting it and/or changing it to one "Currently in use" message. 4) Also, the new admin pages are named "Change XXXX theme". IMO, both the "Change" and the "theme" are really unnecessary. Why not show the device type (surely with a complete "title") and done? That would prevent some warping in the block. 5) Finally, about code, apart of some commas not followed by whitespace, and some "//" comments not followed by whitespace (not sure if this is ruled), it looks correct. So I'm reopening this to think a bit more about the way to avoid the "double UI" thingy together with the rest of minor points. Ciao
          Hide
          Mary Evans added a comment - - edited

          Hi,
          Can I make a suggestion...while these changes are on a backburner?

          The need to change the wording on the Theme selector button came about because of confusion about the 2 tier page system in Theme selector. To the average user, who is used to just selecting a theme in the normal way, finds this changed page view very confusing.

          Can we make the Theme setting option default NO for "Allow mobile device detection" thus making it the responsibility of the Admin to set this to YES if and ONLY IF needed. In my experience on the Themes forum this is one of things which confuses everyone, because it changes the way the Theme selector works.

          If the Theme selector looks like it does normaly in earlier versions 1.9 and 2.0 then when folks update to 2.1.x or 2.2.x they can select their theme in the normal way without going into panic mode. If they then want to make their Moodle site detect Mobile devices they can do so in Theme settings?

          It would also be great if once a theme has been set and the 'Continue' button clicked that the user is sent to the Home page, or at least given an option like on a course page "save and return" or "save and display", rather than just left hanging in the Theme selector.

          I can see all of this needs some discussion.

          Thanks
          Mary

          Show
          Mary Evans added a comment - - edited Hi, Can I make a suggestion...while these changes are on a backburner? The need to change the wording on the Theme selector button came about because of confusion about the 2 tier page system in Theme selector. To the average user, who is used to just selecting a theme in the normal way, finds this changed page view very confusing. Can we make the Theme setting option default NO for "Allow mobile device detection" thus making it the responsibility of the Admin to set this to YES if and ONLY IF needed. In my experience on the Themes forum this is one of things which confuses everyone, because it changes the way the Theme selector works. If the Theme selector looks like it does normaly in earlier versions 1.9 and 2.0 then when folks update to 2.1.x or 2.2.x they can select their theme in the normal way without going into panic mode. If they then want to make their Moodle site detect Mobile devices they can do so in Theme settings? It would also be great if once a theme has been set and the 'Continue' button clicked that the user is sent to the Home page, or at least given an option like on a course page "save and return" or "save and display", rather than just left hanging in the Theme selector. I can see all of this needs some discussion. Thanks Mary
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Eloy and Mary,

          1. I am not sure getting rid of 2-screen UI is back-portable on STABLE branches.
          2. IMHO having two screen UI give user an overview of what theme has been selected for each device, and single point of control for viewing, deleting/unset theme.
          3. Agree, we should not show disabled "Use theme" button
          4. We can add unset theme on Device theme picker page. Probably, we should replace disabled "Use theme" button with unset button
          5. Will change name of the pages with device type only.
          6. "Allow mobile device detection" can be set to no for new installations and for upgrades from 1.9 and 2.0
          7. I think it might be good to have two buttons to have, probably "Change theme" and "Continue", where "Change theme" will redirect user to changing the theme and "continue" should redirect user to home page.

          I will try to have these changes in different commits, so that integrator's can take what they feel is fine.

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Eloy and Mary, I am not sure getting rid of 2-screen UI is back-portable on STABLE branches. IMHO having two screen UI give user an overview of what theme has been selected for each device, and single point of control for viewing, deleting/unset theme. Agree, we should not show disabled "Use theme" button We can add unset theme on Device theme picker page. Probably, we should replace disabled "Use theme" button with unset button Will change name of the pages with device type only. "Allow mobile device detection" can be set to no for new installations and for upgrades from 1.9 and 2.0 I think it might be good to have two buttons to have, probably "Change theme" and "Continue", where "Change theme" will redirect user to changing the theme and "continue" should redirect user to home page. I will try to have these changes in different commits, so that integrator's can take what they feel is fine.
          Hide
          Rajesh Taneja added a comment - - edited

          After taking with Eloy, following will be fixed in this bug (and backported to stable barnches)

          1. Replace string which are creating confusion (original issue)
          2. Add "unset button"
          3. Replace disable "use theme" with "Unset"

          For master I will create another issue, which should take care of:

          1. 2-screen to 1 screen
          2. enhance navigation
          3. "Allow mobile device detection" set to no (by default)
          4. Add two buttons for redirection (after the theme is selected)
          Show
          Rajesh Taneja added a comment - - edited After taking with Eloy, following will be fixed in this bug (and backported to stable barnches) Replace string which are creating confusion (original issue) Add "unset button" Replace disable "use theme" with "Unset" For master I will create another issue, which should take care of: 2-screen to 1 screen enhance navigation "Allow mobile device detection" set to no (by default) Add two buttons for redirection (after the theme is selected)
          Hide
          Rajesh Taneja added a comment -

          Hello Ankit,
          Can you please review this again.
          Following has been fixed:

          1. Strings updated as described in bug
          2. Unset theme button added
          3. Disabled "use theme" button replaced with "unset theme" button (In case on default device no button is displayed)
          4. "Clear theme cache" button will now redirect properly to the page it was pressed.
          Show
          Rajesh Taneja added a comment - Hello Ankit, Can you please review this again. Following has been fixed: Strings updated as described in bug Unset theme button added Disabled "use theme" button replaced with "unset theme" button (In case on default device no button is displayed) "Clear theme cache" button will now redirect properly to the page it was pressed.
          Hide
          Ankit Agarwal added a comment -

          Hi Rajesh,
          Looks good,
          Am not sure but do we allow relative urls?

          new moodle_url('/theme/index.php', array('device' => $device, 'unsettheme' => true, 'sesskey' => sesskey()));
          

          or its better to use $CFG->root instead?
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Rajesh, Looks good, Am not sure but do we allow relative urls? new moodle_url('/theme/index.php', array('device' => $device, 'unsettheme' => true , 'sesskey' => sesskey())); or its better to use $CFG->root instead? Thanks
          Hide
          Mary Evans added a comment - - edited

          @Ankit

          We use new moodle_url a lot in themes rather than $CFG->wwwroot, especially in settings pages, if this helps? It seems that $CFG->wwwroot has been superseded by new moodle_url...but I may be wrong so don't quote me!

          Cheers
          Mary

          Show
          Mary Evans added a comment - - edited @Ankit We use new moodle_url a lot in themes rather than $CFG->wwwroot, especially in settings pages, if this helps? It seems that $CFG->wwwroot has been superseded by new moodle_url...but I may be wrong so don't quote me! Cheers Mary
          Hide
          Mary Evans added a comment - - edited

          Just testing this and think there is still room for improvement.

          Suggestions:

          I think it would look better if we make "Allow device type" in Theme settings default to NO this then would stop the confusion about where to set the default theme. We need to make this User friendly so how about 'Choose your default theme' as the header and (not 'Select default theme')

          This then leaves us free to look at the 4 options where themes may need selecting.

          However, since there is only ONE Mobile theme in CORE I think it would make sense to make that the default for Tablet and Mobile. And since 'Standard legacy' is the only Legacy theme available I feel this should be made the default for Legacy. This leave only the Default theme which is 'Standard'.

          Now this would set the scene for the Administrator who would see at a glance what they need to change (if any), and as it is almost set in stone the only one they need to change would be the Default for their Main site. So perhaps here too we should change Device type default to "Main site theme"

          Also can we put a capital letter in front of Default, Legacy, Tablet, Mobile if nothing other than to make them stand out a little more than they do on the Device selector page?

          Thanks
          Mary

          Show
          Mary Evans added a comment - - edited Just testing this and think there is still room for improvement. Suggestions: I think it would look better if we make "Allow device type" in Theme settings default to NO this then would stop the confusion about where to set the default theme. We need to make this User friendly so how about 'Choose your default theme' as the header and (not 'Select default theme') This then leaves us free to look at the 4 options where themes may need selecting. However, since there is only ONE Mobile theme in CORE I think it would make sense to make that the default for Tablet and Mobile. And since 'Standard legacy' is the only Legacy theme available I feel this should be made the default for Legacy. This leave only the Default theme which is 'Standard'. Now this would set the scene for the Administrator who would see at a glance what they need to change (if any), and as it is almost set in stone the only one they need to change would be the Default for their Main site. So perhaps here too we should change Device type default to "Main site theme" Also can we put a capital letter in front of Default, Legacy, Tablet, Mobile if nothing other than to make them stand out a little more than they do on the Device selector page? Thanks Mary
          Hide
          Mary Evans added a comment - - edited

          What I said before, I forgot you have got this all in hand on different commits...so ignore the most part of it.

          However...here is something I did today playing with CSS3, checkout the images, I have just uploaded, of the Device Theme selector.

          Show
          Mary Evans added a comment - - edited What I said before, I forgot you have got this all in hand on different commits...so ignore the most part of it. However...here is something I did today playing with CSS3, checkout the images, I have just uploaded, of the Device Theme selector.
          Hide
          Mary Evans added a comment -

          Cool eh?

          Show
          Mary Evans added a comment - Cool eh?
          Hide
          Rajesh Taneja added a comment -

          Wow, Looks awesome... Great work Mary

          Thanks for the feedback Mary and Ankit.

          @Ankit: moodle_url internally converts relative url to absolute url, So it's all good.

          @Mary: I agree that core just have one mobile and legacy theme, but if we want to set default theme, then we should consider that for master only. So adding one more task to MDL-31927. Saying so, IMO tablet theme should not be same as mobile theme, so we should not set tablet theme. As per capitalization, sorry it doesn't go with moodle standards, check Helen's comments

          Show
          Rajesh Taneja added a comment - Wow, Looks awesome... Great work Mary Thanks for the feedback Mary and Ankit. @Ankit: moodle_url internally converts relative url to absolute url, So it's all good. @Mary: I agree that core just have one mobile and legacy theme, but if we want to set default theme, then we should consider that for master only. So adding one more task to MDL-31927 . Saying so, IMO tablet theme should not be same as mobile theme, so we should not set tablet theme. As per capitalization, sorry it doesn't go with moodle standards, check Helen's comments
          Hide
          Rajesh Taneja added a comment -

          Submitting for integration review again, with branches re-based.

          Show
          Rajesh Taneja added a comment - Submitting for integration review again, with branches re-based.
          Hide
          Rajesh Taneja added a comment -

          Sorry Mary, Helen just pointed me about capitalization thing. You meant to have it on device selection page and I took it for theme selection page.
          I will fix this and update the branches.

          Thanks Helen and Mary for all the feedback

          Show
          Rajesh Taneja added a comment - Sorry Mary, Helen just pointed me about capitalization thing. You meant to have it on device selection page and I took it for theme selection page. I will fix this and update the branches. Thanks Helen and Mary for all the feedback
          Hide
          Helen Foster added a comment -

          Yes, it all looks very cool Mary!

          Regarding your comment "Also can we put a capital letter in front of Default, Legacy, Tablet, Mobile" I assume it's for on http://tracker.moodle.org/secure/attachment/27254/MDL-27862-DEVICETHEMES.jpg and if so, I agree with you that the words should be capitalized as they're first. (It's when they're in the middle of a phrase that they shouldn't be capitalized.)

          Show
          Helen Foster added a comment - Yes, it all looks very cool Mary! Regarding your comment "Also can we put a capital letter in front of Default, Legacy, Tablet, Mobile" I assume it's for on http://tracker.moodle.org/secure/attachment/27254/MDL-27862-DEVICETHEMES.jpg and if so, I agree with you that the words should be capitalized as they're first. (It's when they're in the middle of a phrase that they shouldn't be capitalized.)
          Hide
          Rajesh Taneja added a comment -

          Thanks Helen,

          Added one more commit to make first letter capital and same font as theme name (under Information)

          Show
          Rajesh Taneja added a comment - Thanks Helen, Added one more commit to make first letter capital and same font as theme name (under Information)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some hours ago...

          the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Rajesh Taneja added a comment -

          Branches re-based.

          Show
          Rajesh Taneja added a comment - Branches re-based.
          Hide
          Aparup Banerjee added a comment -

          thanks for this change, it looks good to me and seems to test fine (not completely tested).

          I've integrated this (functionality) into master, 22 and 21.

          I've backported this functionality to stable branches based on the trivial addition of this functionality (which solves a bug).

          An update to stable+dev docs will be needed.

          Show
          Aparup Banerjee added a comment - thanks for this change, it looks good to me and seems to test fine (not completely tested). I've integrated this (functionality) into master, 22 and 21. I've backported this functionality to stable branches based on the trivial addition of this functionality (which solves a bug). An update to stable+dev docs will be needed.
          Hide
          Michael de Raadt added a comment -

          Test result: Success.

          It was good to see this through to the end.

          Tested under Moodle 2.1, 2.2 and master.

          Show
          Michael de Raadt added a comment - Test result: Success. It was good to see this through to the end. Tested under Moodle 2.1, 2.2 and master.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          FCT (fixed, closing, thanks). Ciao

          "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!"
          ~ Benjamin Disraeli

          Show
          Eloy Lafuente (stronk7) added a comment - FCT (fixed, closing, thanks). Ciao "I feel a very unusual sensation - if it is not indigestion, I think it must be gratitude!" ~ Benjamin Disraeli

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: