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
    • Rank:
      5998

      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: