Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Purge caches to pick up CSS changes
      3. Create a course with 5 sections and set Course layout to Show one section per page
      4. Make sure title of the current section is at the top of the page, in the middle between the navigation links left and right.
      5. Make sure Return to main course page link at the bottom (between the left/right links)

      Note: test this on FF, IE, Chrome and opera

      Show
      Log in as admin Purge caches to pick up CSS changes Create a course with 5 sections and set Course layout to Show one section per page Make sure title of the current section is at the top of the page, in the middle between the navigation links left and right. Make sure Return to main course page link at the bottom (between the left/right links) Note: test this on FF, IE, Chrome and opera
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-32770
    • Rank:
      39767

      Description

      In paged mode:
      "1) The title of the current section should be at the top of the page, in the middle between the navigation links left and right. This looks to be purely a CSS issue for the H2 there.

      2) I think it would be good to add "Return to main course page" as a link between the left/right links at the bottom of a section page. That gives people FOUR ways to get back."

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Hello Dan,

          Shouldn't Return to main course page be *Return to

          {CourseName}* or just *{CourseName}

          * as in breadcrumbs/navigation.

          Show
          Rajesh Taneja added a comment - Hello Dan, Shouldn't Return to main course page be *Return to {CourseName}* or just *{CourseName} * as in breadcrumbs/navigation.
          Hide
          Rajesh Taneja added a comment -

          Adding Martin, to confirm string used for Return to main course
          Should we use

          {Coursename}

          in "Return to main course" link ?

          Show
          Rajesh Taneja added a comment - Adding Martin, to confirm string used for Return to main course Should we use {Coursename} in "Return to main course" link ?
          Hide
          Martin Dougiamas added a comment -

          No, those shortnames can be rather long. And the user is ALREADY in that course, remember.

          "Return to main course page" is still my fav here.

          Show
          Martin Dougiamas added a comment - No, those shortnames can be rather long. And the user is ALREADY in that course, remember. "Return to main course page" is still my fav here.
          Hide
          Rajesh Taneja added a comment -

          Thanks Martin

          Show
          Rajesh Taneja added a comment - Thanks Martin
          Hide
          Dan Poltawski added a comment -

          Looks great ! Exactly what we were after.

          However i've just noticed another problem that needs to be fixed (my fault) if you could in same issue:

          • I used a spacer to seperate the next/previous titles from the arrow. Could you convert that to use CSS and no spacer?

          There is also:

          • Comment typo: pervious
          • Full stop at the end of: // Display section bottom navigation
          Show
          Dan Poltawski added a comment - Looks great ! Exactly what we were after. However i've just noticed another problem that needs to be fixed (my fault) if you could in same issue: I used a spacer to seperate the next/previous titles from the arrow. Could you convert that to use CSS and no spacer? There is also: Comment typo: pervious Full stop at the end of: // Display section bottom navigation
          Hide
          Rajesh Taneja added a comment -

          Thanks Dan,

          Replaced spacer with css and corrected spelling
          Can you please review this again.

          Show
          Rajesh Taneja added a comment - Thanks Dan, Replaced spacer with css and corrected spelling Can you please review this again.
          Hide
          Dan Poltawski added a comment -

          Looks great - pushing the integration button.

          Thanks Raj!

          Show
          Dan Poltawski added a comment - Looks great - pushing the integration button. Thanks Raj!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just 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 - The main moodle.git repository has just 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
          Aparup Banerjee added a comment -

          cool, thats been integrated and ready for test.

          Show
          Aparup Banerjee added a comment - cool, thats been integrated and ready for test.
          Hide
          Ankit Agarwal added a comment -

          Tested this on chromium/FF/Opera and is working fine.

          Show
          Ankit Agarwal added a comment - Tested this on chromium/FF/Opera and is working fine.
          Hide
          Michael de Raadt added a comment -

          This looked consistent for me in FF and IE.

          Show
          Michael de Raadt added a comment - This looked consistent for me in FF and IE.
          Hide
          Ankit Agarwal added a comment -

          Michael tested this on IE, working fine in IE as well.
          Passing.
          Thanks

          Show
          Ankit Agarwal added a comment - Michael tested this on IE, working fine in IE as well. Passing. Thanks
          Hide
          Aparup Banerjee added a comment - - edited

          Hi, just noticed i didn't think about different themes here so i ran a quick test and afterburner seems to be missing the section title.

          ps: guessing the canvas based themes need tweaking?

          Show
          Aparup Banerjee added a comment - - edited Hi, just noticed i didn't think about different themes here so i ran a quick test and afterburner seems to be missing the section title. ps: guessing the canvas based themes need tweaking?
          Hide
          Rajesh Taneja added a comment - - edited

          It's a new feature, not sure if we take care of all themes or theme owner is supposed to upgrade there themes.
          In Afterburner, it has been set to hide.

          .path-course-view .headingblock {
              display: none; /* hides topic items title or weekly itens title */
              margin-bottom: 9px;
          }
          

          Not sure, if we should show title or not.

          Show
          Rajesh Taneja added a comment - - edited It's a new feature, not sure if we take care of all themes or theme owner is supposed to upgrade there themes. In Afterburner, it has been set to hide. .path-course-view .headingblock { display: none; /* hides topic items title or weekly itens title */ margin-bottom: 9px; } Not sure, if we should show title or not.
          Hide
          Martin Dougiamas added a comment -

          We need to upgrade the base themes. In this case I also think fixing canvas would fix most of them.

          Show
          Martin Dougiamas added a comment - We need to upgrade the base themes. In this case I also think fixing canvas would fix most of them.
          Hide
          Rajesh Taneja added a comment -

          Created MDL-33027 to take care of theme compatibility issue. Especially afterburner theme.

          Show
          Rajesh Taneja added a comment - Created MDL-33027 to take care of theme compatibility issue. Especially afterburner theme.
          Hide
          Rajesh Taneja added a comment -

          Hello Apu,

          This has been rectified, is it possible for you to pick 2nd commit "https://github.com/rajeshtaneja/moodle/commit/c42403c834685512a8a4c18feeef2f8a350dfab1" and re-open this for testing?

          Show
          Rajesh Taneja added a comment - Hello Apu, This has been rectified, is it possible for you to pick 2nd commit "https://github.com/rajeshtaneja/moodle/commit/c42403c834685512a8a4c18feeef2f8a350dfab1" and re-open this for testing?
          Hide
          Aparup Banerjee added a comment -

          Hi Raj,
          the 2nd commit uses '!important' to force some styles.

          This will create problems with other themes trying to cascade their styles on top of our base style.
          We have to define our core themes to display correctly but still allow anybody else to style their own theme in their own way.

          i suggest making the style changes needed in base and in the specific theme's core style too. this way other themes (that use base as parent) won't be overridden by the '!important'.

          Show
          Aparup Banerjee added a comment - Hi Raj, the 2nd commit uses '!important' to force some styles. This will create problems with other themes trying to cascade their styles on top of our base style. We have to define our core themes to display correctly but still allow anybody else to style their own theme in their own way. i suggest making the style changes needed in base and in the specific theme's core style too. this way other themes (that use base as parent) won't be overridden by the '!important'.
          Hide
          Sam Hemelryk added a comment -

          Umm !important is really bad (well to the world of theme development at least)

          There has to be a better solution than introducing more !important rules in the base theme.
          We've used them presently only in situations where there has been no other viable solution (of the 4 users in base presently 1 is for ie6 and 2 are for RTL YUI bugs). It would be a shame to start introducing more now.
          They represent a theme nightmare, in order for people to customise things they must recognise that !important rules are being applied and also apply them, in the long run can lead to serious confusion and uncontrollable CSS (because !important is everywhere and there is no way to force things anymore).
          !important is designed to give priority to a CSS selector against its natural order of inheritance, it doesn't mean we should use it to force things, in fact really its better we don't use them at all so that themers if they feel the need can to introduce rules that would not otherwise have priority but still have them applied because through use of !important.
          Certainly my +1 goes to removing all of these new !important rules and correcting the CSS. This will help keep our theme's maintainable in the long run, and help keep things easier for theme developers... unless there is a good reason to keep them I havn't studied it, just noticed the !important properties creeping in.

          Show
          Sam Hemelryk added a comment - Umm !important is really bad (well to the world of theme development at least) There has to be a better solution than introducing more !important rules in the base theme. We've used them presently only in situations where there has been no other viable solution (of the 4 users in base presently 1 is for ie6 and 2 are for RTL YUI bugs). It would be a shame to start introducing more now. They represent a theme nightmare, in order for people to customise things they must recognise that !important rules are being applied and also apply them, in the long run can lead to serious confusion and uncontrollable CSS (because !important is everywhere and there is no way to force things anymore). !important is designed to give priority to a CSS selector against its natural order of inheritance, it doesn't mean we should use it to force things, in fact really its better we don't use them at all so that themers if they feel the need can to introduce rules that would not otherwise have priority but still have them applied because through use of !important. Certainly my +1 goes to removing all of these new !important rules and correcting the CSS. This will help keep our theme's maintainable in the long run, and help keep things easier for theme developers... unless there is a good reason to keep them I havn't studied it, just noticed the !important properties creeping in.
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam and Apu,
          Let me re-visit this then.

          Show
          Rajesh Taneja added a comment - Thanks Sam and Apu, Let me re-visit this then.
          Hide
          Rajesh Taneja added a comment -

          I have removed !important from css. Also added one more commit, which will show hidden section title/link as dimmed-text.

          https://github.com/rajeshtaneja/moodle/commit/4f82488932e67c30ba09982f85c0bdd8b7a558e8
          and
          https://github.com/rajeshtaneja/moodle/commit/8ea356304b6350f6eeea72ad0f286a5b652b3b35

          Adding them to MDl-33027.

          Show
          Rajesh Taneja added a comment - I have removed !important from css. Also added one more commit, which will show hidden section title/link as dimmed-text. https://github.com/rajeshtaneja/moodle/commit/4f82488932e67c30ba09982f85c0bdd8b7a558e8 and https://github.com/rajeshtaneja/moodle/commit/8ea356304b6350f6eeea72ad0f286a5b652b3b35 Adding them to MDl-33027.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Many thanks for the hard work, closing this as fixed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: