Moodle
  1. Moodle
  2. MDL-30213 format properly all CSS in CORE themes
  3. MDL-25512

Correct the CSS rules in Canvas theme text.css to align with YUI3 CSS Fonts

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      Test the following theme's to make sure that the font size is reasonable still:

      • arialist
      • binarius
      • boxxie
      • brick
      • formal_white
      • fusion
      • leatherbound
      • magazine
      • nimble
      • nonzero
      • overlay
      • serenity
      • sky_high
      • splash

      Note this change was made only within the canvas theme - however all the other theme's are based upon canvas. Testing them on the front page and course page should be enough.

      Show
      Test the following theme's to make sure that the font size is reasonable still: arialist binarius boxxie brick formal_white fusion leatherbound magazine nimble nonzero overlay serenity sky_high splash Note this change was made only within the canvas theme - however all the other theme's are based upon canvas. Testing them on the front page and course page should be enough.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-25512_master

      Gliffy Diagrams

        Issue Links

          Activity

          Hide
          Mary Evans added a comment -

          I've just been re-reading the forum discussion started by Urs and lots of MDL came about because of it, but they are all sitting dormant. Of all of them I think this is one which needs attention so I'm going to fix this in Canvas/style/text.css

          Show
          Mary Evans added a comment - I've just been re-reading the forum discussion started by Urs and lots of MDL came about because of it, but they are all sitting dormant. Of all of them I think this is one which needs attention so I'm going to fix this in Canvas/style/text.css
          Hide
          Patrick Malley added a comment -

          Mary, something to consider is whether this text.css file is even necessary if YUI3 CSS has its own font properties. I have very specific tastes when it comes to fonts. Perhaps this text.css file should be in specific themes instead of canvas ....

          Show
          Patrick Malley added a comment - Mary, something to consider is whether this text.css file is even necessary if YUI3 CSS has its own font properties. I have very specific tastes when it comes to fonts. Perhaps this text.css file should be in specific themes instead of canvas ....
          Hide
          Mary Evans added a comment -

          A valid point I agree. HOWEVER, I think the point which Urs was making about the text.css in Canvas theme is worth bearing in mind also, that of not putting the font-family in the body tag, but in the #page.
          To quote Urs...

          "...When in any theme font related definitions are assigned to the body tag the base system breaks - as done in "text.css" in canvas for example. Many advantages using the YUI CSS font rules are thrown away.

          To keep the advantages add font related rules to a subsequent element like "div#page" in all themes..."

          and in relation to font-sizes Urs goes on to say...

          "...By removing the font-size from the body tag and setting the font size in text.css in the canvas theme to "font-size: 108%" for "#page" I verified that the expected 14px font size in the themes are set..."

          This suggest to me that text.css is OK in Canvas theme providing that the css rules are in the correct place. Even if you decided to drop text.css from Canvas, we would still need to make sure the core themes complys with what Urs suggests, that of keeping font styles out of the body tag.

          Show
          Mary Evans added a comment - A valid point I agree. HOWEVER, I think the point which Urs was making about the text.css in Canvas theme is worth bearing in mind also, that of not putting the font-family in the body tag, but in the #page. To quote Urs... "...When in any theme font related definitions are assigned to the body tag the base system breaks - as done in "text.css" in canvas for example. Many advantages using the YUI CSS font rules are thrown away. To keep the advantages add font related rules to a subsequent element like "div#page" in all themes..." and in relation to font-sizes Urs goes on to say... "...By removing the font-size from the body tag and setting the font size in text.css in the canvas theme to "font-size: 108%" for "#page" I verified that the expected 14px font size in the themes are set..." This suggest to me that text.css is OK in Canvas theme providing that the css rules are in the correct place. Even if you decided to drop text.css from Canvas, we would still need to make sure the core themes complys with what Urs suggests, that of keeping font styles out of the body tag.
          Hide
          Urs Hunkler added a comment -

          Mary and Patrick, I exclude text.css from parents in my themes. The best solution may be to remove text.css - but then existing themes may break.

          So the best solution may be to reduce text.css to the bare minimum and care not to override YUI CSS settings.

          Show
          Urs Hunkler added a comment - Mary and Patrick, I exclude text.css from parents in my themes. The best solution may be to remove text.css - but then existing themes may break. So the best solution may be to reduce text.css to the bare minimum and care not to override YUI CSS settings.
          Hide
          Mary Evans added a comment -

          Would making changes in core.css, for all core themes, by taking out any reference to fonts from body tag and move them to #page tag, and doing the same in text.css too, be enough to fix the problem Urs?

          Show
          Mary Evans added a comment - Would making changes in core.css, for all core themes, by taking out any reference to fonts from body tag and move them to #page tag, and doing the same in text.css too, be enough to fix the problem Urs?
          Hide
          Mary Evans added a comment -

          OK, I am revisiting this again today, because I want to fix this BUG 'once and for all' before Moodle 2.1 is released.

          It matters not if I didn't get an answer to my last comment, because on the very same day I left the comment I had the intention of fixing this BUG, but the process for doing Pull Request got changed and as a consequence I got caught up in learning a new way of working and so lost a lot of valuable time. So now I want to make up for that lost time and get this, and other such Moodle BUGS, fixed and in the bag!

          Thanks & Ciao
          Mary

          Show
          Mary Evans added a comment - OK, I am revisiting this again today, because I want to fix this BUG 'once and for all' before Moodle 2.1 is released. It matters not if I didn't get an answer to my last comment, because on the very same day I left the comment I had the intention of fixing this BUG, but the process for doing Pull Request got changed and as a consequence I got caught up in learning a new way of working and so lost a lot of valuable time. So now I want to make up for that lost time and get this, and other such Moodle BUGS, fixed and in the bag! Thanks & Ciao Mary
          Hide
          Mary Evans added a comment - - edited

          Work started on removing font related CSS rules from body tag in all CORE themes and placing them in the div#page tag as suggested by Ure Hunkler, thus enabling YUI CSS rules to work more effectively within Moodle CORE themes.

          Also as part of MDL-25512 I will look at text.css within canvas theme with a view to removing this css file altogether.

          Show
          Mary Evans added a comment - - edited Work started on removing font related CSS rules from body tag in all CORE themes and placing them in the div#page tag as suggested by Ure Hunkler, thus enabling YUI CSS rules to work more effectively within Moodle CORE themes. Also as part of MDL-25512 I will look at text.css within canvas theme with a view to removing this css file altogether.
          Hide
          Sam Hemelryk added a comment -

          Thanks Mary this has been integrated now.
          One thing to remember for next time the MDL in the commit message shouldn't be preceeded by wip-.

          Cheer
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Mary this has been integrated now. One thing to remember for next time the MDL in the commit message shouldn't be preceeded by wip-. Cheer Sam
          Hide
          Michael de Raadt added a comment -

          Tested the listed themes on Windows under Chrome, Firefox and IE. I didn't test the themes that were not listed. For each theme I used the "Use for modern browsers" button.

          In the Fusion theme, the breadcrumbs appeared overlapping the header boundary and is unreadable (I'll attach a screenshot). The font size is OK, but this seems to be a problem that has occurred with this change that needs to be fixed. The problem was consistent across all browsers.

          Show
          Michael de Raadt added a comment - Tested the listed themes on Windows under Chrome, Firefox and IE. I didn't test the themes that were not listed. For each theme I used the "Use for modern browsers" button. In the Fusion theme, the breadcrumbs appeared overlapping the header boundary and is unreadable (I'll attach a screenshot). The font size is OK, but this seems to be a problem that has occurred with this change that needs to be fixed. The problem was consistent across all browsers.
          Hide
          Michael de Raadt added a comment -

          The Fusion theme seems to have been affected by this change. I'm sure it's only a minor issue.

          Show
          Michael de Raadt added a comment - The Fusion theme seems to have been affected by this change. I'm sure it's only a minor issue.
          Hide
          Mary Evans added a comment -

          Thanks Michael, if all the other themes you have tested work OK then that's great, I can fix Fusion and submit fix for integration next week...
          cheers
          Mary

          Show
          Mary Evans added a comment - Thanks Michael, if all the other themes you have tested work OK then that's great, I can fix Fusion and submit fix for integration next week... cheers Mary
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've created MDL-27704 to fix the problems detected in the testing phase.

          So I'm passing this now, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - I've created MDL-27704 to fix the problems detected in the testing phase. So I'm passing this now, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And now... part of upstream, yay, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And now... part of upstream, yay, thanks!
          Hide
          John Stabinger added a comment -

          I added a new issue related to this one to correct for the proper font size:

          http://tracker.moodle.org/browse/MDL-27998

          Should just need to change #page's font-size to 108%...

          Show
          John Stabinger added a comment - I added a new issue related to this one to correct for the proper font size: http://tracker.moodle.org/browse/MDL-27998 Should just need to change #page's font-size to 108%...

            People

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

              Dates

              • Created:
                Updated:
                Resolved: