Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Themes
    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Purge your caches
      3. Check the CSS you browser gets and make sure its normal.
      4. Browse around the site and look at things
      5. Add the following to your config.php
        • $CFG->enablecssoptimiser = true; (This can be set through the UI advanced settings as well if you wish)
        • $CFG->cssoptimiserstats = true;
        • $CFG->cssoptimiserpretty = true;
      6. Purge your caches
      7. Inspect the CSS again and make sure it is smaller but still looks valid
      8. Browse around the site again and make sure things still look the same.
      9. Run unit tests in lib/simpletest/testcsslib.php
      Show
      Log in as admin Purge your caches Check the CSS you browser gets and make sure its normal. Browse around the site and look at things Add the following to your config.php $CFG->enablecssoptimiser = true; (This can be set through the UI advanced settings as well if you wish) $CFG->cssoptimiserstats = true; $CFG->cssoptimiserpretty = true; Purge your caches Inspect the CSS again and make sure it is smaller but still looks valid Browse around the site again and make sure things still look the same. Run unit tests in lib/simpletest/testcsslib.php
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-29941-master
    • Rank:
      19486

      Description

      This issue is to track my progress on creating a new CSS optimiser to organise the mountain of CSS that we have.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment - - edited

          Alright the prototype for the optimiser is up now.
          In short it works - the following is the stats collection from it.

          /****************************************
           *------- CSS Optimisation stats --------
           *  Wed, 26 Oct 2011 18:12:59 +1300
           *  269  	 comments removed
           *  Optimisation took 0.6022 seconds
           *--------------- before ----------------
           *  248424  	 chars read in
           *  2814  	 rules read in
           *  3817  	 total selectors
           *---------------- after ----------------
           *  200150  	 chars once optimized
           *  1652  	 optimized rules
           *  3381  	 total selectors once optimized
           *---------------- stats ----------------
           *  19.4%  	 reduction in chars
           *  41.3%  	 reduction in rules
           *  11.4%  	 reduction in selectors
           ****************************************/
          

          If you're testing the patch make sure you add the following to your config.php (in order to get the state info):

          $CFG->includecssstats = 1;
          

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - - edited Alright the prototype for the optimiser is up now. In short it works - the following is the stats collection from it. /**************************************** *------- CSS Optimisation stats -------- * Wed, 26 Oct 2011 18:12:59 +1300 * 269 comments removed * Optimisation took 0.6022 seconds *--------------- before ---------------- * 248424 chars read in * 2814 rules read in * 3817 total selectors *---------------- after ---------------- * 200150 chars once optimized * 1652 optimized rules * 3381 total selectors once optimized *---------------- stats ---------------- * 19.4% reduction in chars * 41.3% reduction in rules * 11.4% reduction in selectors ****************************************/ If you're testing the patch make sure you add the following to your config.php (in order to get the state info): $CFG->includecssstats = 1; Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Rebased for 2.2 rc1. I will aim to put this up for integration after the release of 2.2 (of shortly before... who knows )

          Show
          Sam Hemelryk added a comment - Rebased for 2.2 rc1. I will aim to put this up for integration after the release of 2.2 (of shortly before... who knows )
          Hide
          Sam Hemelryk added a comment -

          Putting this up for peer-review now.

          Show
          Sam Hemelryk added a comment - Putting this up for peer-review now.
          Hide
          Sam Hemelryk added a comment -

          This is a new experimental feature for Moodle.
          It is a CSS optimiser than can be turned on to replace the use of the minify library for CSS optimisation.
          This optimiser has the advantage that it looks at styles themselves and reorganises them into an optimal form, essentially doing part of the browsers job before caching.
          It provides the following performance changes:

          • The first time CSS cache is generated it takes twice as long (this is once only per upgrade or purge)
          • CSS length reduced by around 20% on average
          • CSS rules reduced by around 40% on average
          • CSS selectors reduced by around 10% on average
          • The above three should provide a good boost to browser performance
          Show
          Sam Hemelryk added a comment - This is a new experimental feature for Moodle. It is a CSS optimiser than can be turned on to replace the use of the minify library for CSS optimisation. This optimiser has the advantage that it looks at styles themselves and reorganises them into an optimal form, essentially doing part of the browsers job before caching. It provides the following performance changes: The first time CSS cache is generated it takes twice as long (this is once only per upgrade or purge) CSS length reduced by around 20% on average CSS rules reduced by around 40% on average CSS selectors reduced by around 10% on average The above three should provide a good boost to browser performance
          Hide
          Andrew Davis added a comment -

          In config-dist.php minamilistic should be minimalistic.

          You describe in the comments in confi-dist.php how the optimization will interact with themedesignermode however I can't see any references to themedesignermode in the new code. Are you sure that the value of themedesignermode will be observed?

          css_store_css() seems to always return true. If the return value is really pointless then don't return anything. If it has value add an @return to the function php docs.

          Consider writing some unit tests for css_is_colour() to prove that those regexs are correct and to define what correct means. And of course to see how it handles invalid values being fed in.

          Similarly, there are a few functions called clean_value(). Some of them like the one in css_style_color are doing some reasonably complicated things. Although they'll be exercised by the higher level tests some lower level ones that test them in isolation could be handy. Not just to test them but to also to describe what they should be doing. Looking at the code the desired output from css_style_color::clean_value() is not clear.

          The php docs @return clause of css_media::organise_rules_by_selectors() doesnt seem to be correct. It says the function returns an array when it appear to return a bool.

          re css_rule::__construct() in its php docs it says "this can only be called from within the scope of this class or its descendants." That is evident from the presence of the protected keyword. Either remove the comment or tell us why (assuming there is an interesting reason).

          +    /**
          +     * Created a new CSS rule. This is the only way to create a new CSS rule externally.
          +     * @return css_rule
          +     */
          +    public static function init() {
          +        return new css_rule();
          +    }
          

          This seems a curious arrangement. Is there a good reason for denying outside classes access to the constructor but then providing an init function that calls it?

          +    /**
          +     * Descreases the current indent
          +     */
          

          Descreases?

          Would it be worth using a modified version of your optimizer to actually overwrite the css we have in git? If there are rules that are being optimized out 100% of the time it would be nice to get them removed from the code base.

          Show
          Andrew Davis added a comment - In config-dist.php minamilistic should be minimalistic. You describe in the comments in confi-dist.php how the optimization will interact with themedesignermode however I can't see any references to themedesignermode in the new code. Are you sure that the value of themedesignermode will be observed? css_store_css() seems to always return true. If the return value is really pointless then don't return anything. If it has value add an @return to the function php docs. Consider writing some unit tests for css_is_colour() to prove that those regexs are correct and to define what correct means. And of course to see how it handles invalid values being fed in. Similarly, there are a few functions called clean_value(). Some of them like the one in css_style_color are doing some reasonably complicated things. Although they'll be exercised by the higher level tests some lower level ones that test them in isolation could be handy. Not just to test them but to also to describe what they should be doing. Looking at the code the desired output from css_style_color::clean_value() is not clear. The php docs @return clause of css_media::organise_rules_by_selectors() doesnt seem to be correct. It says the function returns an array when it appear to return a bool. re css_rule::__construct() in its php docs it says "this can only be called from within the scope of this class or its descendants." That is evident from the presence of the protected keyword. Either remove the comment or tell us why (assuming there is an interesting reason). + /** + * Created a new CSS rule. This is the only way to create a new CSS rule externally. + * @ return css_rule + */ + public static function init() { + return new css_rule(); + } This seems a curious arrangement. Is there a good reason for denying outside classes access to the constructor but then providing an init function that calls it? + /** + * Descreases the current indent + */ Descreases? Would it be worth using a modified version of your optimizer to actually overwrite the css we have in git? If there are rules that are being optimized out 100% of the time it would be nice to get them removed from the code base.
          Hide
          Andrew Davis added a comment -

          This is a huge piece of code that could really help performance. I've listed some points above but they are minor queries really.

          Show
          Andrew Davis added a comment - This is a huge piece of code that could really help performance. I've listed some points above but they are minor queries really.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Thanks for looking at it, I finally found a little more time to work on it.
          I've made most of the changes, cleanups you suggested. In regards to a couple of idea's you had:

          1/ The interaction with theme designer mode.
          This happens automatically and is due to theme designer mode disabling caching. I've added code to the send uncached CSS method to implement the CSS optimiser there as well.
          I've chosen to do this so that theme designers can write piles of CSS, and then use Moodle to optimise it for them and copy+paste the optimised solution.
          It also allows us to inspect the state of individual CSS files.

          2/ Overwriting the CSS in Moodle with the optimised CSS>
          I've considered this but don't think it is a good idea. The CSS we have in Moodle is in a human readable format and although duplicates exist they may be there to help the readability for the theme designer.
          The optimiser pretty much makes things unreadable and almost unordered as it optimised things to shortest solution without changing the ordering of rules unnecessarily.
          I had thought in the future we could analyse the way in which browsers render further and the organise rules to best fit the browsers methods. But I don't have time to look into that now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Thanks for looking at it, I finally found a little more time to work on it. I've made most of the changes, cleanups you suggested. In regards to a couple of idea's you had: 1/ The interaction with theme designer mode. This happens automatically and is due to theme designer mode disabling caching. I've added code to the send uncached CSS method to implement the CSS optimiser there as well. I've chosen to do this so that theme designers can write piles of CSS, and then use Moodle to optimise it for them and copy+paste the optimised solution. It also allows us to inspect the state of individual CSS files. 2/ Overwriting the CSS in Moodle with the optimised CSS> I've considered this but don't think it is a good idea. The CSS we have in Moodle is in a human readable format and although duplicates exist they may be there to help the readability for the theme designer. The optimiser pretty much makes things unreadable and almost unordered as it optimised things to shortest solution without changing the ordering of rules unnecessarily. I had thought in the future we could analyse the way in which browsers render further and the organise rules to best fit the browsers methods. But I don't have time to look into that now. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          A few ideas for how we can further reduce the CSS we are serving:

          • Prevent the last semi-colon in a ruleset from being output
          • Convert font-weight:normal to 400 (cross browser equiviland and 1 char less)
          • Convert font-weight:bold to 700 (cross browser equiviland and 1 char less)
          • Reverse lookup Hex colours for shorter browser colour map equivilants (#F00 => red)
          • Investigate styles with lots of characters and find an method of separating them if it shortens.

          We could also help our designers by identifying suspect CSS rules or rules that can obviously be optimised by removing selectors, or tag names etc

          Show
          Sam Hemelryk added a comment - A few ideas for how we can further reduce the CSS we are serving: Prevent the last semi-colon in a ruleset from being output Convert font-weight:normal to 400 (cross browser equiviland and 1 char less) Convert font-weight:bold to 700 (cross browser equiviland and 1 char less) Reverse lookup Hex colours for shorter browser colour map equivilants (#F00 => red) Investigate styles with lots of characters and find an method of separating them if it shortens. We could also help our designers by identifying suspect CSS rules or rules that can obviously be optimised by removing selectors, or tag names etc
          Hide
          Sam Hemelryk added a comment -

          Ok, I've pushed up one more change that fixes a bunch of things: See the commits for details.
          The short of it is we have a MASSIVE amount of crap invalid CSS in core which I have added tolerance for in the optimiser.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok, I've pushed up one more change that fixes a bunch of things: See the commits for details. The short of it is we have a MASSIVE amount of crap invalid CSS in core which I have added tolerance for in the optimiser. Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          Sounds very good! +1 for the concept!

          Show
          Martin Dougiamas added a comment - Sounds very good! +1 for the concept!
          Hide
          Sam Hemelryk added a comment -

          Crap please don't integrate this, the changes to theme/base/... Should not be there! They must have crept in during work on this (was done largely in my own time bit by bit over time) if you could still please review it but ignore those changes that would be great as it will only take a minute to fix.

          Show
          Sam Hemelryk added a comment - Crap please don't integrate this, the changes to theme/base/... Should not be there! They must have crept in during work on this (was done largely in my own time bit by bit over time) if you could still please review it but ignore those changes that would be great as it will only take a minute to fix.
          Hide
          Aparup Banerjee added a comment -

          no worries Sam, detailed scrutiny coming up (abt 1/3 done)

          Show
          Aparup Banerjee added a comment - no worries Sam, detailed scrutiny coming up (abt 1/3 done)
          Hide
          Sam Hemelryk added a comment -

          Ok I've tidied up the commits now removing the unwanted changes and have pushed the branch back up.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok I've tidied up the commits now removing the unwanted changes and have pushed the branch back up. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Ok I've tidied up the commits now removing the unwanted changes and have pushed the branch back up.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok I've tidied up the commits now removing the unwanted changes and have pushed the branch back up. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Hi Sam,
          Thats a huge patch
          I've noticed a few code issues.
          I've also included what i spotted with the phpdocs as we'll be looking into that in the near future (even though you started writing this way before our docs overhaul)

          /admin/settings/subsystems.php :

          • whitespace right before new setting
          • CnP error: configenableplagiarism string in enablecssoptimiser setting

          /config-dist.php :
          I gathered the English might use a review as its in config-dist.php and we're in a very developer friendly mood with all the docs work going on. I've added Helen too.

          • 'The CSS files the Moodle produces..' : 2nd 'the' -> 'that'
          • '...CSS before it is cached removing excess styles...' -> '...CSS before it is cached thereby removing excess styles...'
          • 'In Moodle 2.2 a CSS optimiser was added' -> that took me for a ride for a bit, nothing in 2.2 already hehe, that should be updated to say 2.3
            *

          /lib/csslib.php :

          • @package core is fine but '@subpackage css' definitely needs to be updated to our latest evolved rules from MDL-30971
          • just noting that this is implemented partly as non-oop
          • css_store_css() needs @throw , the @param(s) needs a brief explanation.
          • 'introduced in Moodle 2.2' should be update to 2.3 , and elsewheres in comments.
          • $optimiser = new css_optimiser; vs new css_optimiser() - i'm not sure of any impacts that this could have but there seems to be this discrepancy in the moodle codebase.
          • css_send_ie_css($themename, $rev)
            • $css assignments missing concatenate operator.
          • css_send_cached_css()
            • (while we're correcting phpdoc grammar) phpdoc description typo : 'generated' instead of 'generate'
            • do we want to hardcode lifetime here? not sure if there's any applicable settings in the codebase either though.
          • css_is_colour()
            • typo in phpdoc , 'Given a value,.. ' , 'regocnised'
            • preg_match('#^(rgb)\s*(\s*(\d {1,3})\s*,\s*(\d{1,3}

              )\s*,\s*(\d

              {1,3}

              )\s*)$#i', $value, $m) - parantheses in the regex do not seem used (performance hit?), also you may want to consider a CRLF after the preg_match calls @,@

          • css_is_width()
            • preg_match parantheses unused apparently.
          • css_optimiser class
            • ok this point onwards i'm skipping phpdocs, assuming you'll look through the rest, will have alook in the next review. This patch is huge so i want to get feedback to u sooner. (idea:perhaps we could use a documentor to generate docs which we can put up separately for review by other readers? - as part of an addition to the current review process)
          • typo : rep 'why re removed'
          • 'continue 3;' -> probably want to reference to the 'for' loop there with a comment. the rest are switches.
          • public static function style -> phpdocs + perhaps discouraging 'important' for devs/debugmode? :-p

          stopping here

          • my ref: reviewed upto class css_selector as per diff against master

          cheers man,
          Apu

          Show
          Aparup Banerjee added a comment - Hi Sam, Thats a huge patch I've noticed a few code issues. I've also included what i spotted with the phpdocs as we'll be looking into that in the near future (even though you started writing this way before our docs overhaul) /admin/settings/subsystems.php : whitespace right before new setting CnP error: configenableplagiarism string in enablecssoptimiser setting /config-dist.php : I gathered the English might use a review as its in config-dist.php and we're in a very developer friendly mood with all the docs work going on. I've added Helen too. 'The CSS files the Moodle produces..' : 2nd 'the' -> 'that' '...CSS before it is cached removing excess styles...' -> '...CSS before it is cached thereby removing excess styles...' 'In Moodle 2.2 a CSS optimiser was added' -> that took me for a ride for a bit, nothing in 2.2 already hehe, that should be updated to say 2.3 * /lib/csslib.php : @package core is fine but '@subpackage css' definitely needs to be updated to our latest evolved rules from MDL-30971 just noting that this is implemented partly as non-oop css_store_css() needs @throw , the @param(s) needs a brief explanation. 'introduced in Moodle 2.2' should be update to 2.3 , and elsewheres in comments. $optimiser = new css_optimiser; vs new css_optimiser() - i'm not sure of any impacts that this could have but there seems to be this discrepancy in the moodle codebase. css_send_ie_css($themename, $rev) $css assignments missing concatenate operator. css_send_cached_css() (while we're correcting phpdoc grammar) phpdoc description typo : 'generated' instead of 'generate' do we want to hardcode lifetime here? not sure if there's any applicable settings in the codebase either though. css_is_colour() typo in phpdoc , 'Given a value,.. ' , 'regocnised' preg_match('#^(rgb)\s*(\s*(\d {1,3})\s*,\s*(\d{1,3} )\s*,\s*(\d {1,3} )\s*)$#i', $value, $m) - parantheses in the regex do not seem used (performance hit?), also you may want to consider a CRLF after the preg_match calls @,@ css_is_width() preg_match parantheses unused apparently. css_optimiser class #@+ phpdoc has recently lost preference, see http://docs.moodle.org/dev/Coding_style#Properties ok this point onwards i'm skipping phpdocs, assuming you'll look through the rest, will have alook in the next review. This patch is huge so i want to get feedback to u sooner. (idea:perhaps we could use a documentor to generate docs which we can put up separately for review by other readers? - as part of an addition to the current review process) typo : rep 'why re removed' 'continue 3;' -> probably want to reference to the 'for' loop there with a comment. the rest are switches. public static function style -> phpdocs + perhaps discouraging 'important' for devs/debugmode? :-p stopping here my ref: reviewed upto class css_selector as per diff against master cheers man, Apu
          Hide
          Sam Hemelryk added a comment -

          Thanks Apu for picking up those docs points and regex things.
          I've just pushed up a branch with an additional commit to fix many of those things.

          In reply to the ones where I didn't fix or there is something worth noting.

          • config-dist.php we can always open up a new issue to look at the wording there. It may be easier to correct that after this has gone in as then we can all look at it with ease and as its master only and just documentation shouldn't be a problem.
          • The non-oop code here is very intentional. All but the css_is_colour/width functions are just a centralisation of functions that already existed and I've simply moved them in. The two that I did introduce I did consider turning into static methods of the colour and width style classes however I figured it was easier to read as they are now.
          • css_store_css() I documented the params, but didn't get what you meant about the @throws. That function doesn't throw exceptions.
          • new css_optimiser; vs new css_optimiser(). That has been debated a couple of times, particularly around the use of stdClass. I am a fan of the no-brackets method as you can tell. The new keyword clearly gives it away as a class instantiation, and the lack of brackets doubly so + it makes it very obvious that there are no instantiation arguments. Previous debates have ended with either way being acceptable.
          • css_is_width All but one of the parenthesis are needed as they are all qualified optional. I've chosen to leave the other one in anyway as it makes it more readable. Parenthesis impact on performance would be negligible anyway I think, and certainly that this only gets run once before caching or when caches are cleared would make it doubly so.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Apu for picking up those docs points and regex things. I've just pushed up a branch with an additional commit to fix many of those things. In reply to the ones where I didn't fix or there is something worth noting. config-dist.php we can always open up a new issue to look at the wording there. It may be easier to correct that after this has gone in as then we can all look at it with ease and as its master only and just documentation shouldn't be a problem. The non-oop code here is very intentional. All but the css_is_colour/width functions are just a centralisation of functions that already existed and I've simply moved them in. The two that I did introduce I did consider turning into static methods of the colour and width style classes however I figured it was easier to read as they are now. css_store_css() I documented the params, but didn't get what you meant about the @throws. That function doesn't throw exceptions. new css_optimiser; vs new css_optimiser(). That has been debated a couple of times, particularly around the use of stdClass. I am a fan of the no-brackets method as you can tell. The new keyword clearly gives it away as a class instantiation, and the lack of brackets doubly so + it makes it very obvious that there are no instantiation arguments. Previous debates have ended with either way being acceptable. css_is_width All but one of the parenthesis are needed as they are all qualified optional. I've chosen to leave the other one in anyway as it makes it more readable. Parenthesis impact on performance would be negligible anyway I think, and certainly that this only gets run once before caching or when caches are cleared would make it doubly so. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Crap forgot to put this up for integration!
          Sneaking it in now. Apu if you'd like to go over it again go for it! otherwise all up to you Eloy!

          Show
          Sam Hemelryk added a comment - Crap forgot to put this up for integration! Sneaking it in now. Apu if you'd like to go over it again go for it! otherwise all up to you Eloy!
          Hide
          Aparup Banerjee added a comment -

          Hi Sam,
          This gets my +1 for integration just from the sheer size and effort here - but - i've just reopened this for your further considerations since you'll be looking into this anyway:

          • css_style_color::clean_value() fails for incorrect colour lengths - https://github.com/nebgor/moodle/commit/711ad5ad57cdbb305b8e5f3bac5c34f9140e13fc <-- commit has unit test that fails - mangles true (albeit borked) css colour.
          • there seems to be a sort of pattern here with regards to usage of 'color' vs 'colour'
            • @param string $color The colour of the border' - this is more confusing than the next point.
            • i can see that functions that are descriptive use 'colour' whereas functions using the css term use 'color'. There is an element of confusion here, do we have a preference ? (ie: function css_is_colour() vs class css_style_color ) - personally its can be potentially be quiet painful for code greppers and the distinction of language vs css term here in csslib.php might not be worth it. (imo)
          • class css_style_* methods have @param needing brief phpdoc explanation - many are $value which could simply use a 'The css integer value' perhaps?
          • class css_style_border* methods
            • @return is incorrect, its returning an array containing the mentioned objects.
          • class css_style_background
            • preg_match('#(none|inherit|url())#', reset($bits)) and operating on $bits as if it were the $matches seems iffy, could use a double check. reset(): 'Returns the value of the first array element, or FALSE if the array is empty.'

          cheers,
          Aparup

          Show
          Aparup Banerjee added a comment - Hi Sam, This gets my +1 for integration just from the sheer size and effort here - but - i've just reopened this for your further considerations since you'll be looking into this anyway: css_style_color::clean_value() fails for incorrect colour lengths - https://github.com/nebgor/moodle/commit/711ad5ad57cdbb305b8e5f3bac5c34f9140e13fc <-- commit has unit test that fails - mangles true (albeit borked) css colour. there seems to be a sort of pattern here with regards to usage of 'color' vs 'colour' @param string $color The colour of the border' - this is more confusing than the next point. i can see that functions that are descriptive use 'colour' whereas functions using the css term use 'color'. There is an element of confusion here, do we have a preference ? (ie: function css_is_colour() vs class css_style_color ) - personally its can be potentially be quiet painful for code greppers and the distinction of language vs css term here in csslib.php might not be worth it. (imo) class css_style_* methods have @param needing brief phpdoc explanation - many are $value which could simply use a 'The css integer value' perhaps? class css_style_border* methods @return is incorrect, its returning an array containing the mentioned objects. class css_style_background preg_match('#(none|inherit|url())#', reset($bits)) and operating on $bits as if it were the $matches seems iffy, could use a double check. reset(): 'Returns the value of the first array element, or FALSE if the array is empty.' cheers, Aparup
          Hide
          Aparup Banerjee added a comment -

          ok scratch the last comment - its the value being passed not as the '$matches' , thanks for clarifying that Sam. I've had an argument mis-alignment error!

          Show
          Aparup Banerjee added a comment - ok scratch the last comment - its the value being passed not as the '$matches' , thanks for clarifying that Sam. I've had an argument mis-alignment error!
          Hide
          Sam Hemelryk added a comment -

          Thanks Apu,
          I've push a branch up now with handling tolerance for 4 and 5 char colours + unit tests to support them.
          In regards to the specific points:

          1. colours are fixed as mentioned and unit tests added
          2. color vs colour: The css optimiser instantiates style objects based upon the name of the style, because the style is color the object and its properties must reflect that. In places where I could I have endeavoured to use colour as per our coding style. I haven't made any changes here to be truthful I don't think we should worry about it... lol confusion at the hands of language difference
          3. Fixed up the phpdocs per the remaining points.
          4. Last point we discussed and has been clarified.

          Back up for integration again.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Apu, I've push a branch up now with handling tolerance for 4 and 5 char colours + unit tests to support them. In regards to the specific points: colours are fixed as mentioned and unit tests added color vs colour: The css optimiser instantiates style objects based upon the name of the style, because the style is color the object and its properties must reflect that. In places where I could I have endeavoured to use colour as per our coding style. I haven't made any changes here to be truthful I don't think we should worry about it... lol confusion at the hands of language difference Fixed up the phpdocs per the remaining points. Last point we discussed and has been clarified. Back up for integration again. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Aparup Banerjee added a comment -

          Sam : config-dist.php -->'2.2' :-p , i forgot, you forgot hehe..

          Show
          Aparup Banerjee added a comment - Sam : config-dist.php -->'2.2' :-p , i forgot, you forgot hehe..
          Hide
          Sam Hemelryk added a comment -

          Thanks Apu - rebased and amended the commit to address the 2.2 in config-dist.php

          Show
          Sam Hemelryk added a comment - Thanks Apu - rebased and amended the commit to address the 2.2 in config-dist.php
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Looks super-great. Thanks for all the iterations up to current status Sam & Apu. And backed with a lot of unit tests, yay! +1 (hope it also works, lol).

          Silly enough, and not preventing integration... wtf is package = core_css and category = css ? They are not one component nor one api. I'm sure Marina's checker will blame you soon.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Looks super-great. Thanks for all the iterations up to current status Sam & Apu. And backed with a lot of unit tests, yay! +1 (hope it also works, lol). Silly enough, and not preventing integration... wtf is package = core_css and category = css ? They are not one component nor one api. I'm sure Marina's checker will blame you soon. Ciao
          Hide
          Aparup Banerjee added a comment - - edited

          ok this has been integrated. nice work Sam

          re: core_css - i'm not sure if we want to update get_core_subsystems() or we want to change the @package but this can be done easily (also when the docs are up for this).

          for now this huge patch has been integrated into master only and is an optional 'subsystem'. It can evolve in there like this or whatever it ends up as.


          ps:

          Also noting that for 'new in master'/experimental stuff, do we want strict documentation and package etc? (maybe '@package experimental'? lol) we can't really support/doc very new stuff and (from what i understand) this will evolve and test itself out there and get refined. (docs_required tag is there)

          hm, perhaps less restrictions on experimental should be allowed? anyway this can be discussed further once CiBoT starts yelling... :-p

          Show
          Aparup Banerjee added a comment - - edited ok this has been integrated. nice work Sam re: core_css - i'm not sure if we want to update get_core_subsystems() or we want to change the @package but this can be done easily (also when the docs are up for this). for now this huge patch has been integrated into master only and is an optional 'subsystem'. It can evolve in there like this or whatever it ends up as. ps: Also noting that for 'new in master'/experimental stuff, do we want strict documentation and package etc? (maybe '@package experimental'? lol) we can't really support/doc very new stuff and (from what i understand) this will evolve and test itself out there and get refined. (docs_required tag is there) hm, perhaps less restrictions on experimental should be allowed? anyway this can be discussed further once CiBoT starts yelling... :-p
          Hide
          Adrian Greeve added a comment -

          The CSS optimiser does indeed reduce the size of the CSS. I checked to make sure that different aspects of the site looked the same before and after the optimiser was enabled.
          I didn't spot any problems.
          Test passed.
          Thanks Sam.

          Show
          Adrian Greeve added a comment - The CSS optimiser does indeed reduce the size of the CSS. I checked to make sure that different aspects of the site looked the same before and after the optimiser was enabled. I didn't spot any problems. Test passed. Thanks Sam.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your nice code represents only 1/46 of the issues that have been sent upstream this week, so thanks, but not many.

          Nah, joking, many thanks! Closing this a fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your nice code represents only 1/46 of the issues that have been sent upstream this week, so thanks, but not many. Nah, joking, many thanks! Closing this a fixed, ciao
          Hide
          Helen Foster added a comment -

          I've created a page in the 2.3 docs for this improvement - http://docs.moodle.org/23/en/CSS_optimiser - and would greatly appreciate help in coming up with documentation for it!

          Show
          Helen Foster added a comment - I've created a page in the 2.3 docs for this improvement - http://docs.moodle.org/23/en/CSS_optimiser - and would greatly appreciate help in coming up with documentation for it!
          Hide
          Helen Foster added a comment -

          Changing label from docs_required to dev_docs_required.

          Show
          Helen Foster added a comment - Changing label from docs_required to dev_docs_required.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: