Moodle
  1. Moodle
  2. MDL-32355

Redundant style selectors in formal_white

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      This issue fixes a parent ordering issue and removes unneeded CSS.
      To test this you just need to test that the format white theme still works.
      Please log in, change your theme to formal_white and have a good play with it.

      Show
      This issue fixes a parent ordering issue and removes unneeded CSS. To test this you just need to test that the format white theme still works. Please log in, change your theme to formal_white and have a good play with it.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32355_master
    • Rank:
      39178

      Description

      Some style selectors are useless in formal_white. They could be inherited from canvas/styles/core.css
      Core.css from canvas theme should be included in order to improve the reliability e maintainability of the theme.

        Activity

        Hide
        Dan Poltawski added a comment -

        Hi Mary,

        I've assigned you as a peer reviewer for this theme change - if you are not able to review it then I will find someone else

        thanks,
        dan

        Show
        Dan Poltawski added a comment - Hi Mary, I've assigned you as a peer reviewer for this theme change - if you are not able to review it then I will find someone else thanks, dan
        Hide
        Mary Evans added a comment - - edited

        I've just written something about Canvas theme in MDL-32356

        The reason the css is not being picked up from Canvas is that you still have the parent themes the wrong way round, they should look like this...

        $THEME->parents = array('canvas', 'base')
        
        Show
        Mary Evans added a comment - - edited I've just written something about Canvas theme in MDL-32356 The reason the css is not being picked up from Canvas is that you still have the parent themes the wrong way round, they should look like this... $THEME->parents = array('canvas', 'base')
        Hide
        Daniele Cordella added a comment -

        no Mary, the css is not being picked up from Canvas because in my config.php I have:

        $THEME->parents_exclude_sheets = array(
            'canvas'=>array(
                'core',
                'pagelayout',
                'tabs',
                'tables',
            ),
        );
        
        Show
        Daniele Cordella added a comment - no Mary, the css is not being picked up from Canvas because in my config.php I have: $THEME->parents_exclude_sheets = array( 'canvas'=>array( 'core', 'pagelayout', 'tabs', 'tables', ), );
        Hide
        Mary Evans added a comment -

        Even so there are other styles in Canvas it can be using, but are getting over written by Base theme because of the order you have your parent themes in the parent theme array.

        If you could put these in the correct order, it would ensure Canvas css is read and used, which will be doubly important if you are erasing all the core css from Formal White and allowing Canvas CORE css to be used. If you don't have the Parent order correct then you run the risk of Base theme overwriting Canvas core.css.

        Hope this make it clearer?

        Show
        Mary Evans added a comment - Even so there are other styles in Canvas it can be using, but are getting over written by Base theme because of the order you have your parent themes in the parent theme array. If you could put these in the correct order, it would ensure Canvas css is read and used, which will be doubly important if you are erasing all the core css from Formal White and allowing Canvas CORE css to be used. If you don't have the Parent order correct then you run the risk of Base theme overwriting Canvas core.css. Hope this make it clearer?
        Hide
        Mary Evans added a comment -

        Further to my last comment,
        When you have made these changes you can Submit this for Integration Review.

        Ciao
        Mary

        Show
        Mary Evans added a comment - Further to my last comment, When you have made these changes you can Submit this for Integration Review. Ciao Mary
        Hide
        Daniele Cordella added a comment -

        submitting for integration as of the comment of Mary at 12/apr/12 4:04 PM... even if I made a mess committing and pushing. I feel (and hope) all is fine now. Thanks all.

        Show
        Daniele Cordella added a comment - submitting for integration as of the comment of Mary at 12/apr/12 4:04 PM... even if I made a mess committing and pushing. I feel (and hope) all is fine now. Thanks all.
        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
        Sam Hemelryk added a comment -

        Hi Daniele, this has been integrated now.

        The commits you made deleted the config.php file and then re-added it, you also made a typo in the second commit.
        To make it clearer what was going on I have tidied up the commits during integration, this will make it clearer to anyone reading history what has changed.

        I'll also add comparative testing instructions to this issue, could you please review them.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Daniele, this has been integrated now. The commits you made deleted the config.php file and then re-added it, you also made a typo in the second commit. To make it clearer what was going on I have tidied up the commits during integration, this will make it clearer to anyone reading history what has changed. I'll also add comparative testing instructions to this issue, could you please review them. Cheers Sam
        Hide
        Daniele Cordella added a comment -

        Ciao Sam. You are right. I made a mess during the push process. I wanted to remove config.php from the stage but I removed it from the hard disk!!!
        Yes I read your testing instructions, thanks. I agree with you, nothing more can be written. Thanks again.

        Show
        Daniele Cordella added a comment - Ciao Sam. You are right. I made a mess during the push process. I wanted to remove config.php from the stage but I removed it from the hard disk!!! Yes I read your testing instructions, thanks. I agree with you, nothing more can be written. Thanks again.
        Hide
        Andrew Davis added a comment -

        Seems to be working fine. Passing.

        Show
        Andrew Davis added a comment - Seems to be working fine. Passing.
        Hide
        Daniele Cordella added a comment -

        Thanks Andrew.

        Show
        Daniele Cordella added a comment - Thanks Andrew.
        Hide
        Dan Poltawski added a comment -

        Bonza mate!

        Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

        Hooroo

        Show
        Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo

          People

          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: