Moodle
  1. Moodle
  2. MDL-38907

Jumpto menu in the frame of the course page display text center aligned

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.3, 2.5
    • Fix Version/s: 2.4.4
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Go to a course.
      2. Set "Course layout" to "Show one section per page"
      3. Go to a course section page.
      4. You are supposed to see the jumpto menu as it is in the attachment: problem.png.
      5. Apply the patch.
      6. You should see the jumpto menu as it is in the attachment: expected.png
      Show
      Go to a course. Set "Course layout" to "Show one section per page" Go to a course section page. You are supposed to see the jumpto menu as it is in the attachment: problem.png. Apply the patch. You should see the jumpto menu as it is in the attachment: expected.png
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38907-master
    • Rank:
      49002

      Description

      Attachments explain the problem better than each word.
      IMHO this fix should be applied to HEAD/course/format/weeks/styles.css but this is a different story.

      1. expected.png
        28 kB
      2. problem.png
        30 kB

        Issue Links

          Activity

          Hide
          Daniele Cordella added a comment -

          I have the fix here for formal_white, but I am busy now. I'll link it later on.
          Ciao.

          Show
          Daniele Cordella added a comment - I have the fix here for formal_white, but I am busy now. I'll link it later on. Ciao.
          Hide
          Daniele Cordella added a comment -

          1) I wrote the testing instruction even if I can not see them in the page.
          I trust they are somewhere.

          2) as I already wrote: I would be happy to see this fix at central level in HEAD/course/format/weeks/styles.css not in formal_white only.
          If you neglect my fix and apply it at central level I'm not offended at all.

          Because of this double working hypothesis, I add Mary here as watcher.

          Show
          Daniele Cordella added a comment - 1) I wrote the testing instruction even if I can not see them in the page. I trust they are somewhere. 2) as I already wrote: I would be happy to see this fix at central level in HEAD/course/format/weeks/styles.css not in formal_white only. If you neglect my fix and apply it at central level I'm not offended at all. Because of this double working hypothesis, I add Mary here as watcher.
          Hide
          Daniele Cordella added a comment - - edited

          Mary, the choice is up to you.

          Show
          Daniele Cordella added a comment - - edited Mary, the choice is up to you.
          Hide
          Mary Evans added a comment -

          My sentiment exactly! This should be in styles.css.

          Show
          Mary Evans added a comment - My sentiment exactly! This should be in styles.css.
          Hide
          Andrea Bicciolo added a comment -

          Looks good to me as per testing instruction.

          Show
          Andrea Bicciolo added a comment - Looks good to me as per testing instruction.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this, everyone.

          Show
          Michael de Raadt added a comment - Thanks for working on this, everyone.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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
          Damyon Wiese added a comment -

          I am stopping the review because I have now rewritten the patch and it needs to be integrated by someone else.

          I just removed the jumpmenu css because I couldn't find it being used anywhere (except on the new menu it was breaking). This will automatically work for RTL.

          See new branch.

          Show
          Damyon Wiese added a comment - I am stopping the review because I have now rewritten the patch and it needs to be integrated by someone else. I just removed the jumpmenu css because I couldn't find it being used anywhere (except on the new menu it was breaking). This will automatically work for RTL. See new branch.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (24 & master), thanks!
          Hide
          Daniele Cordella added a comment -

          Thanks a lot, great Eloy!

          Show
          Daniele Cordella added a comment - Thanks a lot, great Eloy!
          Hide
          Daniele Cordella added a comment -

          Thanks a lot to all the involved staff (without counting Eloy, this time).

          Show
          Daniele Cordella added a comment - Thanks a lot to all the involved staff (without counting Eloy, this time).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          grrr... :-D

          Show
          Eloy Lafuente (stronk7) added a comment - grrr... :-D
          Hide
          Jason Fowler added a comment -

          All fixed

          Show
          Jason Fowler added a comment - All fixed
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your awesome contributions are now part of Moodle, your fav LMS out there.

          Closing this as fixed.

          Many thanks for all the hard work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao
          Hide
          Gareth J Barnard added a comment - - edited

          Hi,

          I know this is closed, but only just seen it and have a few things to point out and ask!

          The '.jumpmenu' css was not needed in Moodle 2.4 as it does not contain the 'Jump menu' code. However, it is used by MDL-34917 (https://github.com/gjb2048/moodle/commit/d46e26f6827d9c50eb8dac901a58833718b754b5) and either needs to be put back in M2.5 or a decision needs to be made that it really is redundant and the code '$select->class = 'jumpmenu';' removed from '/course/format/renderer.php' in a new issue because as this was raised after MDL-34917 was integrated then this should have removed that code too but did not. Thoughts Marina please .

          This is also a 'course' and not 'theme' issue .

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Hi, I know this is closed, but only just seen it and have a few things to point out and ask! The '.jumpmenu' css was not needed in Moodle 2.4 as it does not contain the 'Jump menu' code. However, it is used by MDL-34917 ( https://github.com/gjb2048/moodle/commit/d46e26f6827d9c50eb8dac901a58833718b754b5 ) and either needs to be put back in M2.5 or a decision needs to be made that it really is redundant and the code '$select->class = 'jumpmenu';' removed from '/course/format/renderer.php' in a new issue because as this was raised after MDL-34917 was integrated then this should have removed that code too but did not. Thoughts Marina please . This is also a 'course' and not 'theme' issue . Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          P.S. Aesthetically I think the text should be center aligned on the menu to bring symmetry with the left and right navigation facility.

          Show
          Gareth J Barnard added a comment - P.S. Aesthetically I think the text should be center aligned on the menu to bring symmetry with the left and right navigation facility.
          Hide
          Gareth J Barnard added a comment -

          Raised MDL-39085 to solve this.

          Show
          Gareth J Barnard added a comment - Raised MDL-39085 to solve this.
          Hide
          Gareth J Barnard added a comment -

          P.P.S. I see that the fix branch is MOODLE_24_STABLE only but an examination of the master branch reveals that it has gone into MOODLE_25_STABLE (master) too.

          Show
          Gareth J Barnard added a comment - P.P.S. I see that the fix branch is MOODLE_24_STABLE only but an examination of the master branch reveals that it has gone into MOODLE_25_STABLE (master) too.
          Hide
          Daniele Cordella added a comment -

          Gareth,
          the fix to this issue simply removed two css rules referring to jumpmenu class.
          Why do you say $select->class = 'jumpmenu'; is redundant in /course/format/renderer.php?
          Moodle simply has, in some branch/branches, a not handled css class.
          If you want to manage the jumpmenu menu in your theme, you can add a style rule for it.
          Otherwise, if you are satisfied with it, you can neglect it.

          Did I lighten your doubts? If I didn't, I am not sure I well understand your position and I apologise.

          Show
          Daniele Cordella added a comment - Gareth, the fix to this issue simply removed two css rules referring to jumpmenu class. Why do you say $select->class = 'jumpmenu'; is redundant in /course/format/renderer.php? Moodle simply has, in some branch/branches, a not handled css class. If you want to manage the jumpmenu menu in your theme, you can add a style rule for it. Otherwise, if you are satisfied with it, you can neglect it. Did I lighten your doubts? If I didn't, I am not sure I well understand your position and I apologise.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Daniele Cordella,

          This has nothing to do with themes! But is in fact core course format functionality - not a core or contrib theme issue. I am saying that in MDL-34917 used the class and the screen shots depicted in this issue (if they came from 'master') would not have been possible without MDL-34917.

          Therefore the statement "Moodle simply has, in some branch/branches, a not handled css class." is false because it is being used by the jump menu reimplemented in MDL-34917.

          '/course/format/renderer.php' is a core file which has the code '$select->class = 'jumpmenu';' which references the '.jumpmenu' css definition contained within the 'styles.css' file of the core course formats of 'topics' and 'weeks'.

          Now, it is entirely possible for aesthetic reasons that a 'theme' may choose to implement an override for this style. However, that would imply that either the base theme defines that style or this patch is reverted.

          Course formats perform the layout operation of the functional component of a course and themes style the text, colours etc. So it needs to be decided if the alignment of the jump menu is a course format or theme issue. Comments on MDL-39085 please .

          I hope this all makes sense!

          Kind regards,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Daniele Cordella , This has nothing to do with themes! But is in fact core course format functionality - not a core or contrib theme issue. I am saying that in MDL-34917 used the class and the screen shots depicted in this issue (if they came from 'master') would not have been possible without MDL-34917 . Therefore the statement "Moodle simply has, in some branch/branches, a not handled css class." is false because it is being used by the jump menu reimplemented in MDL-34917 . '/course/format/renderer.php' is a core file which has the code '$select->class = 'jumpmenu';' which references the '.jumpmenu' css definition contained within the 'styles.css' file of the core course formats of 'topics' and 'weeks'. Now, it is entirely possible for aesthetic reasons that a 'theme' may choose to implement an override for this style. However, that would imply that either the base theme defines that style or this patch is reverted. Course formats perform the layout operation of the functional component of a course and themes style the text, colours etc. So it needs to be decided if the alignment of the jump menu is a course format or theme issue. Comments on MDL-39085 please . I hope this all makes sense! Kind regards, Gareth
          Hide
          Mary Evans added a comment -

          You are right Gareth. This should in fact have been adjusted in the Formal White theme and not, in this case, course/style.css. It's my fault this has happened, and so we need to reverse it, but will need a new tracker issue.

          Show
          Mary Evans added a comment - You are right Gareth. This should in fact have been adjusted in the Formal White theme and not, in this case, course/style.css. It's my fault this has happened, and so we need to reverse it, but will need a new tracker issue.
          Hide
          Mary Evans added a comment -

          Moving discussion to MDL-39085 to continue debate. I've spotted the problem. This needs adjusting in course/renderer.php as the jumpmenu class is in form select which is wrong.

          Show
          Mary Evans added a comment - Moving discussion to MDL-39085 to continue debate. I've spotted the problem. This needs adjusting in course/renderer.php as the jumpmenu class is in form select which is wrong.
          Hide
          Daniele Cordella added a comment - - edited

          I am not sure I agree with you.
          I usually consider the style bonded to an html element like an hook.
          You are free to use the hook or not. What is important is that the hook has been created.
          The html item is well formed even if no css file uses the hook to define roles.
          As far as I know, there may be a lot of unhandled/unused html style classes in moodle.

          Show
          Daniele Cordella added a comment - - edited I am not sure I agree with you. I usually consider the style bonded to an html element like an hook. You are free to use the hook or not. What is important is that the hook has been created. The html item is well formed even if no css file uses the hook to define roles. As far as I know, there may be a lot of unhandled/unused html style classes in moodle.
          Hide
          Mary Evans added a comment - - edited

          Daniele, I wish now that I had not said to you to change the course/format style.css for topic and weeks. Looking at it from another viepoint I should have told you to change it in Formal White using

          #sectionmenu .jumpmenu {text-align:left;}

          As it is now, that of removing

           .jumpmenu {text-align:center;}

          may spoil this for others who are happy with the central look of that menu. In some ways I have to agree with Gareth, that it looks better, more aesthetically pleasing.

          If you wanted to accommodate longer titles you could have widened the jumpmenu itself as it is set at 40%, or better still made it width: auto;

          Anyway this issue is closed now, so not point in carrying on with this discussion here.
          If you want to comment then please do so in MDL-39085.

          Show
          Mary Evans added a comment - - edited Daniele, I wish now that I had not said to you to change the course/format style.css for topic and weeks. Looking at it from another viepoint I should have told you to change it in Formal White using #sectionmenu .jumpmenu {text-align:left;} As it is now, that of removing .jumpmenu {text-align:center;} may spoil this for others who are happy with the central look of that menu. In some ways I have to agree with Gareth, that it looks better, more aesthetically pleasing. If you wanted to accommodate longer titles you could have widened the jumpmenu itself as it is set at 40%, or better still made it width: auto; Anyway this issue is closed now, so not point in carrying on with this discussion here. If you want to comment then please do so in MDL-39085 .

            People

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

              Dates

              • Created:
                Updated:
                Resolved: