Moodle
  1. Moodle
  2. MDL-41658

Text in the drop down menu in HEAD/course/index.php?categoryid=1 is center aligned in formal_white (and in all canvas based themes).

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.6, 2.5.2, 2.6
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Themes
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Switch to Formal White theme.
      2. Go to ../course/index.php?categoryid=1 and TEST that the Category dropdown menu options are LEFT aligned and NOT centred.
      3. Also TEST other themes that use Canvas as a parent theme such as:
      • arialist
      • binarius
      • boxxie
      • brick
      • formfactor
      • fusion
      • leatherbound
      • magazine
      • nimble
      • nonzero
      • overlay
      • serenity
      • sky_high
      • splash
      Show
      Switch to Formal White theme. Go to ../course/index.php?categoryid=1 and TEST that the Category dropdown menu options are LEFT aligned and NOT centred. Also TEST other themes that use Canvas as a parent theme such as: arialist binarius boxxie brick formfactor fusion leatherbound magazine nimble nonzero overlay serenity sky_high splash
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-41658_master

      Description

      I found one more centred aligned drop down menu as the one I spotted out in MDL-38907.
      I feel it is really ugly such all the other that I am still missing.
      Here I propose a solution that should fix the issue definitively at general level.

        Gliffy Diagrams

          Activity

          Hide
          Mary Evans added a comment - - edited

          Daniele,

          Although I agree that this is a good idea to change this menu alignment from center to left, however, I do have two issues with your patch for this fix.

          Firstly: it is not necessary to add .ltr for any CSS in Moodle as LTR is the default language.

          Secondly: if you want to make changes like this that are 'global' in nature can I suggest to you that you fix Canvas theme, which is where the actual problem stems from? This then will reduce the amount of CSS duplication that is going on.

          Thanks you
          Mary

          Show
          Mary Evans added a comment - - edited Daniele, Although I agree that this is a good idea to change this menu alignment from center to left, however, I do have two issues with your patch for this fix. Firstly: it is not necessary to add .ltr for any CSS in Moodle as LTR is the default language. Secondly: if you want to make changes like this that are 'global' in nature can I suggest to you that you fix Canvas theme, which is where the actual problem stems from? This then will reduce the amount of CSS duplication that is going on. Thanks you Mary
          Hide
          Daniele Cordella added a comment -

          Ciao Mary
          and thanks for your suggestions.

          I posted a new version of the fix patching canvas/core.css
          About the direction, I am not sure I can drop .dir-ltr and .dir-rtl but, in any case, I love to have them so the sheet is easier to read.

          Can I ask you to peer review this fix and MDL-40262, please?

          Thanks in advance.

          Show
          Daniele Cordella added a comment - Ciao Mary and thanks for your suggestions. I posted a new version of the fix patching canvas/core.css About the direction, I am not sure I can drop .dir-ltr and .dir-rtl but, in any case, I love to have them so the sheet is easier to read. Can I ask you to peer review this fix and MDL-40262 , please? Thanks in advance.
          Hide
          Mary Evans added a comment -

          Starting Peer Review

          Show
          Mary Evans added a comment - Starting Peer Review
          Hide
          Mary Evans added a comment -

          Ciao Daniele,

          You only need to remove .singleselect from line 39 of theme/canvas/style/core.css for this to be a global fix.

          https://github.com/moodle/moodle/blob/master/theme/canvas/style/core.css#L39

          Show
          Mary Evans added a comment - Ciao Daniele, You only need to remove .singleselect from line 39 of theme/canvas/style/core.css for this to be a global fix. https://github.com/moodle/moodle/blob/master/theme/canvas/style/core.css#L39
          Hide
          Mary Evans added a comment -

          Also it is not necessary to add changes to class .select as this is NOT needed in core css.

          If you want to add that change to Formal White you can do IF and ONLY IF it is needed.
          Please test first after you remove .singleselect from Canvas theme as mentioned above. This should fix the centering of the lists.

          Show
          Mary Evans added a comment - Also it is not necessary to add changes to class .select as this is NOT needed in core css. If you want to add that change to Formal White you can do IF and ONLY IF it is needed. Please test first after you remove .singleselect from Canvas theme as mentioned above. This should fix the centering of the lists.
          Hide
          Daniele Cordella added a comment -

          Coool Mary.
          You are 150% right.
          I am always afraid to touch code I did not write but, of course, your solution is much better than mine.
          I changed once again the patch as of your suggestion.
          I submit for peer review waiting for your news.

          Will I meet you in Mediterranean Moodle Moot?

          Show
          Daniele Cordella added a comment - Coool Mary. You are 150% right. I am always afraid to touch code I did not write but, of course, your solution is much better than mine. I changed once again the patch as of your suggestion. I submit for peer review waiting for your news. Will I meet you in Mediterranean Moodle Moot?
          Hide
          Mary Evans added a comment -

          Ciao Daniele,

          The code is OK now but, you have too many commits which confuse the Integratore.

          Can I suggest you create a new branch based on Master for this and delete the other branch to keep it clean?

          Than can you also create branches to fix MOODLE_24_STABLE and MOODLE_25_STABLE too? You can then set this for Integration Review.

          Show
          Mary Evans added a comment - Ciao Daniele, The code is OK now but, you have too many commits which confuse the Integratore. Can I suggest you create a new branch based on Master for this and delete the other branch to keep it clean? Than can you also create branches to fix MOODLE_24_STABLE and MOODLE_25_STABLE too? You can then set this for Integration Review.
          Hide
          Daniele Cordella added a comment -

          Mary, I think I did all the necessary for an happy coming integration.
          If you find something wrong, do not hesitate to write down whatever is needed.
          Ciao.

          Show
          Daniele Cordella added a comment - Mary, I think I did all the necessary for an happy coming integration. If you find something wrong, do not hesitate to write down whatever is needed. Ciao.
          Hide
          Mary Evans added a comment -

          +100!!!
          meraviglioso grazie

          buona notte!

          Show
          Mary Evans added a comment - +100!!! meraviglioso grazie buona notte!
          Hide
          Sam Hemelryk added a comment -

          Thanks Mary - this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Mary - this has been integrated now.
          Hide
          Andrew Davis added a comment -

          Works as described. Passing.

          Show
          Andrew Davis added a comment - Works as described. Passing.
          Hide
          Mary Evans added a comment -

          Excellent - many thanks everyone!

          Show
          Mary Evans added a comment - Excellent - many thanks everyone!
          Hide
          Damyon Wiese added a comment -

          This issue along with 77 of it's friends has been sent upstream and released to the world.

          Thankyou for your contribution.

          Show
          Damyon Wiese added a comment - This issue along with 77 of it's friends has been sent upstream and released to the world. Thankyou for your contribution.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: