Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.7
    • Fix Version/s: 2.7
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Switch to More theme
      2. Edit theme/more/less/moodle.less and add some LESS rules. Example: @textColor: red;
      3. Browse Moodle
      4. The LESS rules should be reflected in real time (if not in designer mode you might need to purge your cache)

      Repeat on IE < 10, and IE >= 10
      Repeat with themedesignermode disabled/enabled

      Show
      Switch to More theme Edit theme/more/less/moodle.less and add some LESS rules. Example: @textColor: red; Browse Moodle The LESS rules should be reflected in real time (if not in designer mode you might need to purge your cache) Repeat on IE < 10, and IE >= 10 Repeat with themedesignermode disabled/enabled
    • Affected Branches:
      MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-44357-master-noplugin
    • Sprint:
      FRONTEND Sprint 11
    • Sprint:
      FRONTEND Sprint 11

      Description

      Implement the ability for themes to compile LESS from PHP

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            fred Frédéric Massart added a comment -

            Sending that for peer review. Please note that this is blocked by some other issues in order to work properly. I just created that issue to organise the work to be done.

            Show
            fred Frédéric Massart added a comment - Sending that for peer review. Please note that this is blocked by some other issues in order to work properly. I just created that issue to organise the work to be done.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-44357

            • Remote repository: git://github.com/FMCorz/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-44357 Remote repository: git://github.com/FMCorz/moodle.git Remote branch MDL-44357 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1765 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1765/artifact/work/smurf.html
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Just trying this out quickly to try adding sourceMap support.

            Adding initial sourcemap support seems pretty easy and I have a first go working already, but that's including the sourceMap inline in the content. What we really need to do is:

            • write the sourceMap to a file (preferably next to where we write the CSS file);
            • set a sourceMapURL in the config to point to something which serves this file.

            Voila.

            I can do this hackily (and have as a Proof of Concept), but the compiler code is called in css_content_from_less(), but rather than saving the file directly it's returning the content so that it can be saved in css_files_from_less().

            However, I'm seeing the less -> css generation taking in the region of 15 seconds (both before, and after the sourcemap changes).

            Show
            dobedobedoh Andrew Nicols added a comment - Just trying this out quickly to try adding sourceMap support. Adding initial sourcemap support seems pretty easy and I have a first go working already, but that's including the sourceMap inline in the content. What we really need to do is: write the sourceMap to a file (preferably next to where we write the CSS file); set a sourceMapURL in the config to point to something which serves this file. Voila. I can do this hackily (and have as a Proof of Concept), but the compiler code is called in css_content_from_less(), but rather than saving the file directly it's returning the content so that it can be saved in css_files_from_less(). However, I'm seeing the less -> css generation taking in the region of 15 seconds (both before, and after the sourcemap changes).
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Fred,

            I see from: https://github.com/FMCorz/moodle/compare/692d247...MDL-44357-master#diff-cc9d797a974977b9ce7716a8e802799dR4 - that you are importing all of the files. In my experience in doing this manually 'variables.less' and others are not found as the compiler needs to be in the 'bootstrapbase/less' folder and not 'theme/less' folder unless you add a path parameter. Without looking at depth in the code, are you doing this?

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Fred, I see from: https://github.com/FMCorz/moodle/compare/692d247...MDL-44357-master#diff-cc9d797a974977b9ce7716a8e802799dR4 - that you are importing all of the files. In my experience in doing this manually 'variables.less' and others are not found as the compiler needs to be in the 'bootstrapbase/less' folder and not 'theme/less' folder unless you add a path parameter. Without looking at depth in the code, are you doing this? Gareth
            Hide
            gb2048 Gareth J Barnard added a comment - - edited

            Hi Andrew,

            Source maps can be generated with the lessc compiler using that method. As in the bootstrap grunt build file: https://github.com/bmbrands/theme_bootstrap/blob/master/Gruntfile.js#L64. But are only of use if compression is not used and theme designer mode is on. And don't create a separate file but use completely inline in the same file. A lot less bother!

            All: Will this be an option here?

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - - edited Hi Andrew, Source maps can be generated with the lessc compiler using that method. As in the bootstrap grunt build file: https://github.com/bmbrands/theme_bootstrap/blob/master/Gruntfile.js#L64 . But are only of use if compression is not used and theme designer mode is on. And don't create a separate file but use completely inline in the same file. A lot less bother! All: Will this be an option here? Gareth
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Gareth,

            If you try the patch that Fred has produced, you should see that it works.

            I've got the sourcemaps working too. Sourcemaps should use regardless of compression, but we only want them to run when theme designer mode is enabled.

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Gareth, If you try the patch that Fred has produced, you should see that it works. I've got the sourcemaps working too. Sourcemaps should use regardless of compression, but we only want them to run when theme designer mode is enabled. Andrew
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Andrew,

            Thanks. I've not had a chance to try the patch. Was just a question based upon observation.

            However with manual source map compilation with v1.6+ of the lessc compiler under Node.js I've found that when inline with the output file i.e. 'lessc --source-map-map-inline --source-map-less-inline --source-map-rootpath=xxxxxx' then adding compression does not work (does not give the right value) when debugging in Chrome.

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Andrew, Thanks. I've not had a chance to try the patch. Was just a question based upon observation. However with manual source map compilation with v1.6+ of the lessc compiler under Node.js I've found that when inline with the output file i.e. 'lessc --source-map-map-inline --source-map-less-inline --source-map-rootpath=xxxxxx' then adding compression does not work (does not give the right value) when debugging in Chrome. Gareth
            Hide
            fred Frédéric Massart added a comment -

            Thanks guys,

            sourceMap is something that I haven't used yet, so for the sake of having this issue integrated asap I would delay its implementation. I will raise an issue about it though.

            Gareth, I do not just copy/paste the content into a new file, I sometimes play with @import and use a relative path to the theme LESS file. So it works just the way you're expecting it.

            I have rebased the patch on top of Petr's work in the linked issue, and added a locking mechanism to prevent multiple requests to start building the file again and kill a server.

            Peer reviews are welcome.

            Fred
            PS: themedesignermode is slow, that's a fact. I am not sure how to solve this just yet...

            Show
            fred Frédéric Massart added a comment - Thanks guys, sourceMap is something that I haven't used yet, so for the sake of having this issue integrated asap I would delay its implementation. I will raise an issue about it though. Gareth, I do not just copy/paste the content into a new file, I sometimes play with @import and use a relative path to the theme LESS file. So it works just the way you're expecting it. I have rebased the patch on top of Petr's work in the linked issue, and added a locking mechanism to prevent multiple requests to start building the file again and kill a server. Peer reviews are welcome. Fred PS: themedesignermode is slow, that's a fact. I am not sure how to solve this just yet...
            Hide
            bawjaws David Scotson added a comment -

            The less compiler has a cache for some of it's intermediate stages, from a look at the diff it doesn't seem like you're using it yet. That would be the first place I'd look for speedups.

            https://github.com/oyejorge/less.php#parser-caching

            I think the leafo one had the ability to skip compilation completely if the source files hadn't changed, not sure if the new one does too. That's obviously a big win in the theme developer mode case.

            Show
            bawjaws David Scotson added a comment - The less compiler has a cache for some of it's intermediate stages, from a look at the diff it doesn't seem like you're using it yet. That would be the first place I'd look for speedups. https://github.com/oyejorge/less.php#parser-caching I think the leafo one had the ability to skip compilation completely if the source files hadn't changed, not sure if the new one does too. That's obviously a big win in the theme developer mode case.
            Hide
            bawjaws David Scotson added a comment -

            Oh, and there's an unrelated commit (fixing some broken CSS) in the diff.

            Show
            bawjaws David Scotson added a comment - Oh, and there's an unrelated commit (fixing some broken CSS) in the diff.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Frédéric Massart,

            Oh I did not mean that I copy / paste. Its just that the upstream bootstrap has import statements without paths, so the compiler needs them as parameters to its path in order to find them.

            I've found theme designer mode slow. I performed a code walk through of how themes are loaded into memory via index.php and discovered that the theme structures are loaded twice. The first time to perform some validation. Then the entire contents are dumped. Then they are loaded again. I also found that the theme config.php files are read two to three times per theme in the hierarchy per page load. So for 'Clean' this is 'Clean', 'Bootstrapbase' and 'Base'. The latter for the 'layouts' array. Off the top of my head you can see this in the log with something like 'error_log(print_r(debug_backtrace(), true));'. I'm sure I've mentioned this on the theme's forum or a tracker issue in the past somewhere.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Frédéric Massart , Oh I did not mean that I copy / paste. Its just that the upstream bootstrap has import statements without paths, so the compiler needs them as parameters to its path in order to find them. I've found theme designer mode slow. I performed a code walk through of how themes are loaded into memory via index.php and discovered that the theme structures are loaded twice. The first time to perform some validation. Then the entire contents are dumped. Then they are loaded again. I also found that the theme config.php files are read two to three times per theme in the hierarchy per page load. So for 'Clean' this is 'Clean', 'Bootstrapbase' and 'Base'. The latter for the 'layouts' array. Off the top of my head you can see this in the log with something like 'error_log(print_r(debug_backtrace(), true));'. I'm sure I've mentioned this on the theme's forum or a tracker issue in the past somewhere. Cheers, Gareth
            Hide
            fred Frédéric Massart added a comment -

            Hi guys

            David Scotson, the parser from Leafo was terrible, and should not be used when playing with a little bit more than just Bootstrap out-of-the-box. I quickly tried to implement the caching mechanism, but for now I will leave it on the side as I didn't see any result and have limited time to work on it. I will raise an issue for that.

            Gareth J Barnard, no, the compiler does not need the paths to resolve them, it will resolve them the same way LESS does it, relatively from the file being parsed. I think you've spotted an unrelated performance issue, would you mind raison an issue for it?

            Thanks!
            Fred

            Show
            fred Frédéric Massart added a comment - Hi guys David Scotson , the parser from Leafo was terrible, and should not be used when playing with a little bit more than just Bootstrap out-of-the-box. I quickly tried to implement the caching mechanism, but for now I will leave it on the side as I didn't see any result and have limited time to work on it. I will raise an issue for that. Gareth J Barnard , no, the compiler does not need the paths to resolve them, it will resolve them the same way LESS does it, relatively from the file being parsed. I think you've spotted an unrelated performance issue, would you mind raison an issue for it? Thanks! Fred
            Hide
            skodak Petr Skoda added a comment -

            Hi, two issues in the theme/styles.php

            1/ while (false === ($lock = $lockfactory->get_lock($themename, 20))) { - you do not need to verify the lock, all we need here is to wait some time to prevent server overloading

            2/ on the line with the following you should release the lock - // The file was built while we waited for the lock.

            Show
            skodak Petr Skoda added a comment - Hi, two issues in the theme/styles.php 1/ while (false === ($lock = $lockfactory->get_lock($themename, 20))) { - you do not need to verify the lock, all we need here is to wait some time to prevent server overloading 2/ on the line with the following you should release the lock - // The file was built while we waited for the lock.
            Hide
            fred Frédéric Massart added a comment -

            Thanks Petr, this is fixed now.

            Show
            fred Frédéric Massart added a comment - Thanks Petr, this is fixed now.
            Hide
            skodak Petr Skoda added a comment -

            Hi, I cannot comment much about the actual less compilation, but the changes in the existing API look reasonable and encapsulated enough so that we may improve it later if necessary.

            Show
            skodak Petr Skoda added a comment - Hi, I cannot comment much about the actual less compilation, but the changes in the existing API look reasonable and encapsulated enough so that we may improve it later if necessary.
            Hide
            skodak Petr Skoda added a comment -

            +1

            Show
            skodak Petr Skoda added a comment - +1
            Hide
            fred Frédéric Massart added a comment -

            Thanks Petr, pushing for integration.

            Some of the things this patch includes:

            1. Compiling LESS in a theme
              • 3 new theme variables introduced. exclude_from_parents_with_less to allow a sensible fallback on standard CSS.
              • parents LESS files are ignored. The LESS file of the theme should be self contained, so that it can be compiled using standard LESS softwares if need be.
            2. Plugins can provide a LESS file, if not a CSS file is looked for.
            3. IE < 10 chunks the file in theme designer mode
            4. Some other commits fix existing CSS that caused LESS to break. I noticed recess is less strict than other compilers.
            5. I plan to write some documentation summarising all this.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Thanks Petr, pushing for integration. Some of the things this patch includes: Compiling LESS in a theme 3 new theme variables introduced. exclude_from_parents_with_less to allow a sensible fallback on standard CSS. parents LESS files are ignored. The LESS file of the theme should be self contained, so that it can be compiled using standard LESS softwares if need be. Plugins can provide a LESS file, if not a CSS file is looked for. IE < 10 chunks the file in theme designer mode Some other commits fix existing CSS that caused LESS to break. I noticed recess is less strict than other compilers. I plan to write some documentation summarising all this. Cheers, Fred
            Hide
            bawjaws David Scotson added a comment -

            "the parser from Leafo was terrible, and should not be used when playing with a little bit more than just Bootstrap out-of-the-box"

            Actually, it doesn't even work 100% for that case, that was one of the reasons I stopped looking into it.

            But the general principle of only doing the work when you need to is sound. Currently when using LESS that supplies the output to Moodle as CSS you can hook grunt up so that it watches your less files, notices changes you make in an editor, it then regenerates the CSS, then it call a PHP function to clear the caches so the new CSS gets picked up on the next page change, so you get the benefits of Theme Developer Mode, without every page load having to do the work unecessarily.

            Show
            bawjaws David Scotson added a comment - "the parser from Leafo was terrible, and should not be used when playing with a little bit more than just Bootstrap out-of-the-box" Actually, it doesn't even work 100% for that case, that was one of the reasons I stopped looking into it. But the general principle of only doing the work when you need to is sound. Currently when using LESS that supplies the output to Moodle as CSS you can hook grunt up so that it watches your less files, notices changes you make in an editor, it then regenerates the CSS, then it call a PHP function to clear the caches so the new CSS gets picked up on the next page change, so you get the benefits of Theme Developer Mode, without every page load having to do the work unecessarily.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi David,

            We did consider that, but we cannot guarantee that nodejs will be available on the target system in order to recompile. We'd also have to write an additional file out with the current settings supplied to the theme in order to be able to compile with everything in place (e.g. colours).

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi David, We did consider that, but we cannot guarantee that nodejs will be available on the target system in order to recompile. We'd also have to write an additional file out with the current settings supplied to the theme in order to be able to compile with everything in place (e.g. colours). Andrew
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Can I ask innocently why do we need this is core? Wouldn't be better to move it under the composer umbrella a provide the functionality from there? Easier updates, lighter distro...

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Can I ask innocently why do we need this is core? Wouldn't be better to move it under the composer umbrella a provide the functionality from there? Easier updates, lighter distro... Ciao
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Eloy Lafuente (stronk7),

            It's because we need less.php to be included in the distribution - we cannot rely upon composer being present to install it. This issue isn't about starting to build with a different tool, but having the less file build into css on the fly on an installation of Moodle so that we can then allow customisation of the bootstrap variables from the UI - shiny

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Eloy Lafuente (stronk7) , It's because we need less.php to be included in the distribution - we cannot rely upon composer being present to install it. This issue isn't about starting to build with a different tool, but having the less file build into css on the fly on an installation of Moodle so that we can then allow customisation of the bootstrap variables from the UI - shiny Andrew
            Hide
            bushido Mark Nielsen added a comment -

            Just my 2 cents on moving this to composer. I think it is very reasonable to add this to composer and expect developers to install dependencies. Pretty much every project I see these days is either installing dependencies with composer or is installed by composer. Composer also takes care of autoloading the 3rd party code which can vary a lot from project to project.

            Regarding distribution: the Moodle packages in the downloads could go through a build process and download dependencies. That way if someone downloads Moodle from moodle.org, they do not need to run composer.

            Just food for thought as I feel like this will just come up again when another 3rd party lib is included. Cheers!

            Show
            bushido Mark Nielsen added a comment - Just my 2 cents on moving this to composer. I think it is very reasonable to add this to composer and expect developers to install dependencies. Pretty much every project I see these days is either installing dependencies with composer or is installed by composer. Composer also takes care of autoloading the 3rd party code which can vary a lot from project to project. Regarding distribution: the Moodle packages in the downloads could go through a build process and download dependencies. That way if someone downloads Moodle from moodle.org, they do not need to run composer. Just food for thought as I feel like this will just come up again when another 3rd party lib is included. Cheers!
            Hide
            stronk7 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
            stronk7 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
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Again... I'm not able to completely buy this approach. With old themes we have been able to "inject" colors without much problem. With less ones we need to put a compiler on everybody hands? I really don't get the point. If this is progress then something is really wrong in Progressland.

            I'd accept it may be required if adding "custom less" to a theme (and still would force sites wanting any customization to have something extra installed). But just for variable replacements...

            Not stopping this at all, just giving my opinion.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Again... I'm not able to completely buy this approach. With old themes we have been able to "inject" colors without much problem. With less ones we need to put a compiler on everybody hands? I really don't get the point. If this is progress then something is really wrong in Progressland. I'd accept it may be required if adding "custom less" to a theme (and still would force sites wanting any customization to have something extra installed). But just for variable replacements... Not stopping this at all, just giving my opinion.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi,

            Eloy makes a good point.

            I see that from a user's point of view compiling LESS is like scaling Everest on a pogo stick. However from a theme designers point of view its relatively trivial. Therefore, once created and adapted, a theme is a static entity. A theme designer will add in functionality to change certain elements as a part of the theme's appeal to the user for an off the shelf strategy and offer support for bespoke customisation.

            Having delved deeply into the markup / CSS mapping in Clean etc. I see the real issue as being more of structure, design and duplication. If a 'define once use many' approach was consistently applied then you would not need to recompile the LESS, but rather just override a single class.

            So I'm all for progress, but what gap in the market does this functionality fill?

            Like Eloy, just thoughts.

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi, Eloy makes a good point. I see that from a user's point of view compiling LESS is like scaling Everest on a pogo stick. However from a theme designers point of view its relatively trivial. Therefore, once created and adapted, a theme is a static entity. A theme designer will add in functionality to change certain elements as a part of the theme's appeal to the user for an off the shelf strategy and offer support for bespoke customisation. Having delved deeply into the markup / CSS mapping in Clean etc. I see the real issue as being more of structure, design and duplication. If a 'define once use many' approach was consistently applied then you would not need to recompile the LESS, but rather just override a single class. So I'm all for progress, but what gap in the market does this functionality fill? Like Eloy, just thoughts. Gareth
            Hide
            bawjaws David Scotson added a comment -

            I'm not particularly sure this is a good idea either, however I'd dispute the idea that "with old themes we have been able to "inject" colors without much problem". The color stuff in less solves real problems and the bootswatch-based themes demonstrates the power of this.

            However, I personally would be happier if .less remained as a decoupled theme-building tool that delivered its output to Moodle as plain CSS, rather than be built right into Moodle itself. In an ideal world Moodle wouldn't require a less compilation step, because the core HTML would all conform to modern industry standards and so would just work with Bootstrap CSS. I consider the fact that it does currently need less to do some translation to be a bug, but that bug might get deprioritized and forgotten about if less gets embedded into core.

            Show
            bawjaws David Scotson added a comment - I'm not particularly sure this is a good idea either, however I'd dispute the idea that "with old themes we have been able to "inject" colors without much problem". The color stuff in less solves real problems and the bootswatch-based themes demonstrates the power of this. However, I personally would be happier if .less remained as a decoupled theme-building tool that delivered its output to Moodle as plain CSS, rather than be built right into Moodle itself. In an ideal world Moodle wouldn't require a less compilation step, because the core HTML would all conform to modern industry standards and so would just work with Bootstrap CSS. I consider the fact that it does currently need less to do some translation to be a bug, but that bug might get deprioritized and forgotten about if less gets embedded into core.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Sorry Fred - it really looks like this needs more discussion.

            Several very valid points of view have been expressed here (thanks everyone) and to me it seems that no one is too sure that this is the right thing to do quite yet.

            In regards to the code things look pretty good, the only thing I really noted so far was in lib/outputlib.php: All theme config properties before your change are public, the properties you've introduced are protected. I'm much rather see them being made public to achieve consistency.

            My thoughts on this change, at the moment I'm really not sold on it for a few reasons:

            1. First up this seems to be super duper fragile as is shown by c1b99f7 (grader report fix). That is obviously a shitty bit of CSS, but CSS is a fault tollerant language and this really highlights an issue (to me) with the current approach. It just seems obviously wrong moving to a system that requires the designer to inspect web server logs should they have a CSS bug. No doubt such an issue comes because you're passing CSS and less through the less processor rather than just less. Requiring CSS to conform to less standards seems like a wrong move.
            So - less is a strict version of CSS. I think if we're going to push css through a less processor we should require the processor to "ignore" non-strict CSS, the same way a browser does, so that the result is consistent.
            Anyway I hope you get the drift, I think I explained that horribly sorry!

            2. Processing time for the CSS, testing with the clean theme CSS generation time was 12s+ with theme designer mode on. That is much longer than the 4.2s+ I was experiencing before this change. While I know it is unavoidable, PHP being slow etc,etc it does bring to mind the idea of introducing build dependencies on less, or perhaps having an option to use less via the system rather than less via php.
            I don't know really, the time increase is really unfortunate as theme designer mode is really vital when working on a theme, we've encouraged themers to use it, and here we are doubling the time it takes to display a page. It feels like a very unfortunate performance loss, albeit one that only affects designers/developers.

            3. One of the great things about theme designer mode was that it used to deliver each CSS file individually, which when working on a theme allowed me to very easily find the location of something I wished to tinker with.
            Now, with less compilation we end up with just one large file, and no current way of easily finding the style.
            Of course having a sourcemap is the solution to this. I can see it was discussed earlier and you decided it wasn't urgent to the task however I feel differently. I would have called that a feature loss, and something that I would really miss (of course I don't design themes on a regular basis, I just develop) but still you break my workflow and there's not even a linked issue at present.
            I would (personally) require either sourcemaps to come with this change, or to see an issue created and code to implement sourcemaps on it, so that I could test it. Otherwise I can't be sure its possible and without it this would be another stone in bucket against this change.

            4. I really don't get plugins being able to have less files, but that we don't ALWAYS load less files. Surely I'm missing something because if not that will have a disasterous impact should some unsuspecting plugin developer see that it is possible to use less, and not realise it is only to compliment whats in styles.css.

            5. Getting to the nitty gritty points here, there is really no explanation or discussion on this issue about why this change is wanted, why it needs to be in PHP, what we want less for etc.
            I don't doubt you've thought about it, and discussed it, it would be really useful to have more details in this issues description.
            Is MDL-44364 the sole reason for this change? What other things could this be used for? Will we start letting plugins define less stylesheets? how does that work actually?
            If this is for variable replacement, just like Eloy I would suggest really digging for other solutions. (by the way I see that browsers are starting to implement CSS browser variable functionality, ff29, chrome sometime soon, and IE... well who knows, we couldn't rely on this of course but interesting to note).

            6. Mark, Gareth, David, and Eloy have all shared really great perspectives here. I really think we need to look closely at the points raised.

            7. Perhaps a bug, noting just so that I/someone check it out before this lands. When I apply the patch the first page load never seems to work. The styles don't get delivered. There are no errors in the logs and nothing visible in the page obviously. When I refresh the problem is fixed.

            So... a few points there but really all about discussion.

            I should note I really like less, and David is 101% right, it solves some real world problems. I'm just not convinced we should burden Moodle with this power and complexity, which is very much a part of the theme design process rather than the web application process, all for the sake of having an easily customisable colour set for a theme (surely there are other solutions, like conditional settings on system processes or something).
            As I see things at the moment this would get a -1 from me sorry, but I am looking forward to seeing how the discussion goes and perhaps changing my mind.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Sorry Fred - it really looks like this needs more discussion. Several very valid points of view have been expressed here (thanks everyone) and to me it seems that no one is too sure that this is the right thing to do quite yet. In regards to the code things look pretty good, the only thing I really noted so far was in lib/outputlib.php: All theme config properties before your change are public, the properties you've introduced are protected. I'm much rather see them being made public to achieve consistency. My thoughts on this change, at the moment I'm really not sold on it for a few reasons: 1. First up this seems to be super duper fragile as is shown by c1b99f7 (grader report fix). That is obviously a shitty bit of CSS, but CSS is a fault tollerant language and this really highlights an issue (to me) with the current approach. It just seems obviously wrong moving to a system that requires the designer to inspect web server logs should they have a CSS bug. No doubt such an issue comes because you're passing CSS and less through the less processor rather than just less. Requiring CSS to conform to less standards seems like a wrong move. So - less is a strict version of CSS. I think if we're going to push css through a less processor we should require the processor to "ignore" non-strict CSS, the same way a browser does, so that the result is consistent. Anyway I hope you get the drift, I think I explained that horribly sorry! 2. Processing time for the CSS, testing with the clean theme CSS generation time was 12s+ with theme designer mode on. That is much longer than the 4.2s+ I was experiencing before this change. While I know it is unavoidable, PHP being slow etc,etc it does bring to mind the idea of introducing build dependencies on less, or perhaps having an option to use less via the system rather than less via php. I don't know really, the time increase is really unfortunate as theme designer mode is really vital when working on a theme, we've encouraged themers to use it, and here we are doubling the time it takes to display a page. It feels like a very unfortunate performance loss, albeit one that only affects designers/developers. 3. One of the great things about theme designer mode was that it used to deliver each CSS file individually, which when working on a theme allowed me to very easily find the location of something I wished to tinker with. Now, with less compilation we end up with just one large file, and no current way of easily finding the style. Of course having a sourcemap is the solution to this. I can see it was discussed earlier and you decided it wasn't urgent to the task however I feel differently. I would have called that a feature loss, and something that I would really miss (of course I don't design themes on a regular basis, I just develop) but still you break my workflow and there's not even a linked issue at present. I would (personally) require either sourcemaps to come with this change, or to see an issue created and code to implement sourcemaps on it, so that I could test it. Otherwise I can't be sure its possible and without it this would be another stone in bucket against this change. 4. I really don't get plugins being able to have less files, but that we don't ALWAYS load less files. Surely I'm missing something because if not that will have a disasterous impact should some unsuspecting plugin developer see that it is possible to use less, and not realise it is only to compliment whats in styles.css. 5. Getting to the nitty gritty points here, there is really no explanation or discussion on this issue about why this change is wanted, why it needs to be in PHP, what we want less for etc. I don't doubt you've thought about it, and discussed it, it would be really useful to have more details in this issues description. Is MDL-44364 the sole reason for this change? What other things could this be used for? Will we start letting plugins define less stylesheets? how does that work actually? If this is for variable replacement, just like Eloy I would suggest really digging for other solutions. (by the way I see that browsers are starting to implement CSS browser variable functionality, ff29, chrome sometime soon, and IE... well who knows, we couldn't rely on this of course but interesting to note). 6. Mark, Gareth, David, and Eloy have all shared really great perspectives here. I really think we need to look closely at the points raised. 7. Perhaps a bug, noting just so that I/someone check it out before this lands. When I apply the patch the first page load never seems to work. The styles don't get delivered. There are no errors in the logs and nothing visible in the page obviously. When I refresh the problem is fixed. So... a few points there but really all about discussion. I should note I really like less, and David is 101% right, it solves some real world problems. I'm just not convinced we should burden Moodle with this power and complexity, which is very much a part of the theme design process rather than the web application process, all for the sake of having an easily customisable colour set for a theme (surely there are other solutions, like conditional settings on system processes or something). As I see things at the moment this would get a -1 from me sorry, but I am looking forward to seeing how the discussion goes and perhaps changing my mind. Cheers Sam
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Sam,

            Wow, what a reply!

            Slightly off topic but related to your comment. The theme loading process is slow because the mechanism loads the theme and its parents (including base regardless of config.php), validates them, then loses the entire data structure from memory, then repeats the whole process again to load the theme. A structured walk through of the code starting with the main 'index.php' file will demonstrate this assertion, along with putting a stack trace log statement in the theme's config.php file (all of them, i.e. Clean, Bootstrapbase and Base) will show how many times the file is loaded.

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Sam, Wow, what a reply! Slightly off topic but related to your comment. The theme loading process is slow because the mechanism loads the theme and its parents (including base regardless of config.php), validates them, then loses the entire data structure from memory, then repeats the whole process again to load the theme. A structured walk through of the code starting with the main 'index.php' file will demonstrate this assertion, along with putting a stack trace log statement in the theme's config.php file (all of them, i.e. Clean, Bootstrapbase and Base) will show how many times the file is loaded. Gareth
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            fred Frédéric Massart added a comment -

            Hi everyone,

            thank you for your time on this issue. Let me explain a bit more why this was developed.

            As you guys have understood, the one and only goal was to have more settings to customize Clean from the UI (MDL-43786). And we could think of a few possible ways to achieve this.

            Placeholders in LESS

            Like other themes do, we would have a post_process() callback that replaces the colour values, or image paths.

            Placeholders would have to be placed in the LESS files (to generate a CSS containing them):

            • Either as a value for variables... @textColor: [[theme/color]]
            • Or replacing any occurrence of variables... e.g. @textColor by [[theme/color]].

            But that is not acceptable for 2 reasons. Firstly we would eventually need to hack the original Boostrap files to add the placeholders, but more importantly, LESS plays with the colours to generate others with mixins. darken() is commonly used and would fail at compile. And even if we found a way to make that work, a standalone LESS compiler would easily fail (in some circumstances, because I know that placeholders for images work...)

            Simpler post_process

            Another solution, still with post_process() is to work the same way famous themes do. Having a massive set of CSS in the theme itself, overriding every rule that has colours from bootstrapbase, with rules with placeholders everywhere.

            I did not go for that solution because that simply means either:

            • Having just a little set of rules, that will not cover Moodle entirely and thus make fake promises, and have inconsistent styles.
            • Creating a full theme, and I'm pretty sure that's not what we want.

            Compiling LESS

            After investigating both previous solutions, we were left with the most powerful one, compiling LESS and forget about archaic CSS solutions. Again, here there were 2 possibles solution, compiling from an external dependency, or from PHP. But as recess or lessc are both depending node.js, it didn't seem to be a sensible solution for 'Allowing more settings in Clean' as it requires so much dependencies that not everyone can afford. So the PHP solution was investigated.

            Firstly, I played around with lessphp, which was sort of fast but unfortunately very broken. So I then tried less.php, which is a port of lessc. It is working very well, but is slow.

            I see a lot of comments here, and some very interesting, but let's not forget that the goal of this is to provide a way, for an admin, to quickly style its theme to have a site that looks like their school's. Designers should actually not use this feature because they can run LESS and provide a full theme.

            It is also right to say that if Moodle was using sensible CSS classes everywhere, it would make it much simpler for everyone to create a theme, even in pure CSS. But that is not the case, and unfortunately this will take years to be achieved.

            Answering Sam's review:

            1/ Yes it is now strict and solid, but I personally do not see that as a problem. Broken CSS is broken CSS and does not work, it should either be fixed or removed. It has to be considered as a bug, the same way we fix 'undefined notices' in PHP, even though it handles them very well 99% of the time. But because I did not want to break the site when LESS failed, there is a fallback on the regular CSS styles.

            2/ Yes, it is extremely slow and that really annoys me too. I have tried to find way to reduce the time needed to compile, and thought about different approaches but none of them really worked: MDL-44453. The reason I pushed this issue for integration before I fix that, was because I do not consider the slowness of the compiler to be a blocker of the system itself.

            3/ If you keep in mind the goal of the issue, compiling LESS is enough to get settings in the UI. Source maps are a nice to have feature, but so far only some developers are using them and most do not even know they exist. I understand that this is interesting, and Andrew had them work, and my +100 to add support for it, but this is not blocking the goal I am trying to achieve here.

            4/ I thought that allowing LESS files in plugins was a good idea, but maybe I was wrong. The logic I introduced was to use a LESS file if it existed, else to use the CSS one. And when I think about it I am becoming less and less convinced it was a good idea. Especially that it forces everyone to have a styles.css file even though they have a styles.less one. The reasoning was that, if you want to use colours in your plugins, instead of arbitraty choosing one, you'd use the right one. But maybe I was trying to solve something the wrong way. Surely an element library would solve that the right way.

            5/ We have a serious problem when it comes to validate/approve projects. That was part of the planning we had after 2.6 release, which included many others things too. That was also discussed during Scrum, and I think that I tried to follow the approach that seemed the best to achieve the goal I had. While this discussion was not really made public (but not hidden either), you can follow this link to find the original thoughts we had months ago: https://docs.google.com/document/d/1ktkigIsJrxcqPeHWcDdRANA1072JVZZ0azg0INo6v48/edit#heading=h.s3ovaozhzke0 - About the browsers new features, I didn't know about that, but that is definitely something that we won't be able to rely on until 2016... IE \o/

            6/ I hope I have answered most of those in all my comments above.

            7/ I did not encounter this issue, I actually tried install and upgrade and did not face any issue either.

            Now, my after-feedback thoughts:

            It seems that we are contesting 2 things here really:

            1. It is slow.
            2. It is a whole library for only a bunch of colours.

            Well, yes it is slow, but perhaps we can make it so that is does not affect anyone but the admin that changes a setting. If the theme that uses LESS is not used by developers or designers, then what is the problem having a 10s page load whenever the theme cache is purged? So we could have 'Clean' as default, and 'StyleMe' which is a very, very, very basic theme, containing nothing but the added settings and the ability to compile LESS. Thus causing 0 problems to developers and designers.

            I think compiling LESS from PHP is the solution until I'm proven that another solution would work for the goal given. And so the library is required, but as we already have a 200MB package, I am not afraid of adding a library to it.

            Now because I'm being honest, here is one more stone to hit me with : Because less.php is a port, it does not support the latest less.js features. Perhaps integrators should compile the LESS files using a CLI working with less.php to make sure that it does not encounter any compile error. Though I do not consider that as a blocker either.

            Thanks all for the constructive feedback, I regret that it happens only now, but unfortunately I am not convinced that without a status Waiting for integration this would have gathered much feedback sooner...

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi everyone, thank you for your time on this issue. Let me explain a bit more why this was developed. As you guys have understood, the one and only goal was to have more settings to customize Clean from the UI ( MDL-43786 ). And we could think of a few possible ways to achieve this. Placeholders in LESS Like other themes do, we would have a post_process() callback that replaces the colour values, or image paths. Placeholders would have to be placed in the LESS files (to generate a CSS containing them): Either as a value for variables... @textColor: [ [theme/color] ] Or replacing any occurrence of variables... e.g. @textColor by [ [theme/color] ]. But that is not acceptable for 2 reasons. Firstly we would eventually need to hack the original Boostrap files to add the placeholders, but more importantly, LESS plays with the colours to generate others with mixins. darken() is commonly used and would fail at compile. And even if we found a way to make that work, a standalone LESS compiler would easily fail (in some circumstances, because I know that placeholders for images work...) Simpler post_process Another solution, still with post_process() is to work the same way famous themes do. Having a massive set of CSS in the theme itself, overriding every rule that has colours from bootstrapbase, with rules with placeholders everywhere. I did not go for that solution because that simply means either: Having just a little set of rules, that will not cover Moodle entirely and thus make fake promises, and have inconsistent styles. Creating a full theme, and I'm pretty sure that's not what we want. Compiling LESS After investigating both previous solutions, we were left with the most powerful one, compiling LESS and forget about archaic CSS solutions. Again, here there were 2 possibles solution, compiling from an external dependency, or from PHP. But as recess or lessc are both depending node.js, it didn't seem to be a sensible solution for 'Allowing more settings in Clean' as it requires so much dependencies that not everyone can afford. So the PHP solution was investigated. Firstly, I played around with lessphp , which was sort of fast but unfortunately very broken. So I then tried less.php , which is a port of lessc . It is working very well, but is slow. — I see a lot of comments here, and some very interesting, but let's not forget that the goal of this is to provide a way, for an admin, to quickly style its theme to have a site that looks like their school's. Designers should actually not use this feature because they can run LESS and provide a full theme. It is also right to say that if Moodle was using sensible CSS classes everywhere, it would make it much simpler for everyone to create a theme, even in pure CSS. But that is not the case, and unfortunately this will take years to be achieved. — Answering Sam's review: 1/ Yes it is now strict and solid, but I personally do not see that as a problem. Broken CSS is broken CSS and does not work, it should either be fixed or removed. It has to be considered as a bug, the same way we fix 'undefined notices' in PHP, even though it handles them very well 99% of the time. But because I did not want to break the site when LESS failed, there is a fallback on the regular CSS styles. 2/ Yes, it is extremely slow and that really annoys me too. I have tried to find way to reduce the time needed to compile, and thought about different approaches but none of them really worked: MDL-44453 . The reason I pushed this issue for integration before I fix that, was because I do not consider the slowness of the compiler to be a blocker of the system itself. 3/ If you keep in mind the goal of the issue, compiling LESS is enough to get settings in the UI. Source maps are a nice to have feature, but so far only some developers are using them and most do not even know they exist. I understand that this is interesting, and Andrew had them work, and my +100 to add support for it, but this is not blocking the goal I am trying to achieve here. 4/ I thought that allowing LESS files in plugins was a good idea, but maybe I was wrong. The logic I introduced was to use a LESS file if it existed, else to use the CSS one. And when I think about it I am becoming less and less convinced it was a good idea. Especially that it forces everyone to have a styles.css file even though they have a styles.less one. The reasoning was that, if you want to use colours in your plugins, instead of arbitraty choosing one, you'd use the right one. But maybe I was trying to solve something the wrong way. Surely an element library would solve that the right way. 5/ We have a serious problem when it comes to validate/approve projects. That was part of the planning we had after 2.6 release, which included many others things too. That was also discussed during Scrum, and I think that I tried to follow the approach that seemed the best to achieve the goal I had. While this discussion was not really made public (but not hidden either), you can follow this link to find the original thoughts we had months ago: https://docs.google.com/document/d/1ktkigIsJrxcqPeHWcDdRANA1072JVZZ0azg0INo6v48/edit#heading=h.s3ovaozhzke0 - About the browsers new features, I didn't know about that, but that is definitely something that we won't be able to rely on until 2016... IE \o/ 6/ I hope I have answered most of those in all my comments above. 7/ I did not encounter this issue, I actually tried install and upgrade and did not face any issue either. — Now, my after-feedback thoughts: It seems that we are contesting 2 things here really: It is slow. It is a whole library for only a bunch of colours. Well, yes it is slow, but perhaps we can make it so that is does not affect anyone but the admin that changes a setting. If the theme that uses LESS is not used by developers or designers, then what is the problem having a 10s page load whenever the theme cache is purged? So we could have 'Clean' as default, and 'StyleMe' which is a very, very, very basic theme, containing nothing but the added settings and the ability to compile LESS. Thus causing 0 problems to developers and designers. I think compiling LESS from PHP is the solution until I'm proven that another solution would work for the goal given. And so the library is required, but as we already have a 200MB package, I am not afraid of adding a library to it. Now because I'm being honest, here is one more stone to hit me with : Because less.php is a port, it does not support the latest less.js features. Perhaps integrators should compile the LESS files using a CLI working with less.php to make sure that it does not encounter any compile error. Though I do not consider that as a blocker either. Thanks all for the constructive feedback, I regret that it happens only now, but unfortunately I am not convinced that without a status Waiting for integration this would have gathered much feedback sooner... Cheers, Fred
            Hide
            bawjaws David Scotson added a comment -

            Regarding alternative approaches to MDL-43786 (Add some Clean customization settings from the UI), an incremental approach would be to have a child theme of Clean (or a setting that adds more CSS to the Clean theme), with some extra CSS that has color placeholders of the standard [[Moodle]] type.

            This solution isn't infinitely flexible, in the manner that compiling a bootswatch is, but would work for what most people require, i.e. change the banner color at the top, the banner text color, the link color (possibly the btn-primary color to match), and maybe add a logo to the banner, while leaving the majority of Clean (e.g. dark gray text on a white background) intact. And that could then be expanded, bit by bit from there, while still having something useful to ship, basically from day one.

            I'm fairly certain I actually did this already and posted it to the theme forum, though I can't find it right now. I distinctly remember having to specify multiple colors in the gradients that Bootstrap uses for e.g. the bar at the top. I started with a nicely formatted copy of the Clean CSS and deleted any line that didn't have "color" on it to start it off, then figured out which colors where actually slighly lightened/darkened versions of others (I just cheated and specified those different shades of the colors separately, though some fancy PHP could calculate it for you from the core color).

            This is a small amount of duplicate CSS, but if I recall correctly it wasn't particularly bad since it was only the color related lines. Probably worth it for the value people would get from it (and bigger, higher traffic places can generate an actual custom theme if they need it).

            Show
            bawjaws David Scotson added a comment - Regarding alternative approaches to MDL-43786 (Add some Clean customization settings from the UI), an incremental approach would be to have a child theme of Clean (or a setting that adds more CSS to the Clean theme), with some extra CSS that has color placeholders of the standard [ [Moodle] ] type. This solution isn't infinitely flexible, in the manner that compiling a bootswatch is, but would work for what most people require, i.e. change the banner color at the top, the banner text color, the link color (possibly the btn-primary color to match), and maybe add a logo to the banner, while leaving the majority of Clean (e.g. dark gray text on a white background) intact. And that could then be expanded, bit by bit from there, while still having something useful to ship, basically from day one. I'm fairly certain I actually did this already and posted it to the theme forum, though I can't find it right now. I distinctly remember having to specify multiple colors in the gradients that Bootstrap uses for e.g. the bar at the top. I started with a nicely formatted copy of the Clean CSS and deleted any line that didn't have "color" on it to start it off, then figured out which colors where actually slighly lightened/darkened versions of others (I just cheated and specified those different shades of the colors separately, though some fancy PHP could calculate it for you from the core color). This is a small amount of duplicate CSS, but if I recall correctly it wasn't particularly bad since it was only the color related lines. Probably worth it for the value people would get from it (and bigger, higher traffic places can generate an actual custom theme if they need it).
            Hide
            dougiamas Martin Dougiamas added a comment -

            OK, so I had another chat with Fred this afternoon and I think we have a way forward. Much of it has been said above but I want to re-phrase it.

            Some fixed points of reference that affect decisions:
            1) Moodle has a complex HTML structure which means any theming requires lots of tweaks
            2) Developing on Moodle code (even not on themes) can not be slow (eg over 10 seconds per page)
            3) We NEED one core theme in Moodle that provides unskilled/lazy admins with a basic spectrum of options out of the box

            The proposed solution we have is this:

            1) Minimal:

            • Create a new theme that is completely minimalist, and gets back to the original purpose of Clean, which is to express bootstrapbase as simply as possible. This would be like Clean is now out of the box but with no settings at all.
            • I think "Minimal" would work as a name too, it's available.
            • Maintenance of this theme should be almost nothing.
            • This theme will be perfect for development and testing.
            • This themes will also be a well-documented barebones theme people can copy to start their own (using their own LESS strategies).

            2) Clean:

            • Keep it pretty much as already planned for 2.7, with automatic LESS compilation when settings are changed, basic color settings and so on. However, keep it completely self-contained.
            • This means no .less files in plugins. The reason for this is that using .less in plugins rapidly gets very complicated in future, and we are already planning to reduce/eliminate most plugin-specific CSS with the element library over time. This means that for now the various plugins-specific hacks remain in the theme. It also means that in designer mode we keep the separate files that Sam mentioned.
            • This also means the lessc library can also go into the theme itself instead of /lib.
            • We no longer recommend that people copy this theme as an example.

            I think this solves most of the main issues people are having here, while still retaining the advantages.

            I'm still not sure about whether Clean or Minimal would be default (on a new install they would look the same anyway). I'm tending towards Clean.

            As we only have one more sprint (3 weeks) before code freeze, it would be good to have some consensus on this very soon.

            Show
            dougiamas Martin Dougiamas added a comment - OK, so I had another chat with Fred this afternoon and I think we have a way forward. Much of it has been said above but I want to re-phrase it. Some fixed points of reference that affect decisions: 1) Moodle has a complex HTML structure which means any theming requires lots of tweaks 2) Developing on Moodle code (even not on themes) can not be slow (eg over 10 seconds per page) 3) We NEED one core theme in Moodle that provides unskilled/lazy admins with a basic spectrum of options out of the box The proposed solution we have is this: 1) Minimal: Create a new theme that is completely minimalist, and gets back to the original purpose of Clean, which is to express bootstrapbase as simply as possible. This would be like Clean is now out of the box but with no settings at all. I think "Minimal" would work as a name too, it's available. Maintenance of this theme should be almost nothing. This theme will be perfect for development and testing. This themes will also be a well-documented barebones theme people can copy to start their own (using their own LESS strategies). 2) Clean: Keep it pretty much as already planned for 2.7, with automatic LESS compilation when settings are changed, basic color settings and so on. However, keep it completely self-contained. This means no .less files in plugins . The reason for this is that using .less in plugins rapidly gets very complicated in future, and we are already planning to reduce/eliminate most plugin-specific CSS with the element library over time. This means that for now the various plugins-specific hacks remain in the theme. It also means that in designer mode we keep the separate files that Sam mentioned. This also means the lessc library can also go into the theme itself instead of /lib. We no longer recommend that people copy this theme as an example. I think this solves most of the main issues people are having here, while still retaining the advantages. I'm still not sure about whether Clean or Minimal would be default (on a new install they would look the same anyway). I'm tending towards Clean. As we only have one more sprint (3 weeks) before code freeze, it would be good to have some consensus on this very soon.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Instead of causing a documentation regression between M2.6 and M2.7 with all the places and guides that mention 'Clean' and its cloning, I consider that it would be far better and less work to keep 'Clean' as it is and not replace with 'Minimal'. Also the definition of 'Clean' would not really match what is intended to be added to it. Therefore this new LESS based theme should have another new name such as 'Custom' which I believe has not been used - https://moodle.org/plugins/browse.php?list=category&id=3. 'Custom' would also conceptually match the definition of being able to customise the theme.

            With regards 'less' files in plugins. Then I think they should be allowed only in respect as the source of the compiled CSS that provides the additional styling required. Such and example is Collapsed Topics which even with an element library I would still need some of it's own CSS to support its functionality.

            Show
            gb2048 Gareth J Barnard added a comment - Instead of causing a documentation regression between M2.6 and M2.7 with all the places and guides that mention 'Clean' and its cloning, I consider that it would be far better and less work to keep 'Clean' as it is and not replace with 'Minimal'. Also the definition of 'Clean' would not really match what is intended to be added to it. Therefore this new LESS based theme should have another new name such as 'Custom' which I believe has not been used - https://moodle.org/plugins/browse.php?list=category&id=3 . 'Custom' would also conceptually match the definition of being able to customise the theme. With regards 'less' files in plugins. Then I think they should be allowed only in respect as the source of the compiled CSS that provides the additional styling required. Such and example is Collapsed Topics which even with an element library I would still need some of it's own CSS to support its functionality.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Thanks Gareth.

            You can still provide .css files with plugins, as now, but they should be pretty minimal and I don't think (but I could be wrong) that .less files really help so much, do they? Anyhow, the main point is that we won't be including them in themes.

            I do get what you mean about Clean but it's already got quite a bit of code added on top of bootstrap even in 2.6 ... which keeps the difference between it and Custom less clear. That's what gave me a very slight edge on going the other way.

            I do like the Custom name though, would also work for me. What do others think?

            Show
            dougiamas Martin Dougiamas added a comment - Thanks Gareth. You can still provide .css files with plugins, as now, but they should be pretty minimal and I don't think (but I could be wrong) that .less files really help so much, do they? Anyhow, the main point is that we won't be including them in themes. I do get what you mean about Clean but it's already got quite a bit of code added on top of bootstrap even in 2.6 ... which keeps the difference between it and Custom less clear. That's what gave me a very slight edge on going the other way. I do like the Custom name though, would also work for me. What do others think?
            Hide
            gb2048 Gareth J Barnard added a comment -

            No worries Martin.

            Pragmatically using LESS in plugins is overkill like the Monty Python Mosquito hunting sketch. I now know LESS and cannot see how it can help in my course format plugins.

            I don't actually think that Clean has a lot of code on top of Bootstrapbase, just four settings which are applied though one library function in the layout files / simple CSS pre-processing and the standard logo file serving code. Not much at all really. Perhaps if the navbar inversion and the footnote were removed leaving just the logo and custom CSS then that would provide the bare minimal that would be wanted.

            Show
            gb2048 Gareth J Barnard added a comment - No worries Martin. Pragmatically using LESS in plugins is overkill like the Monty Python Mosquito hunting sketch. I now know LESS and cannot see how it can help in my course format plugins. I don't actually think that Clean has a lot of code on top of Bootstrapbase, just four settings which are applied though one library function in the layout files / simple CSS pre-processing and the standard logo file serving code. Not much at all really. Perhaps if the navbar inversion and the footnote were removed leaving just the logo and custom CSS then that would provide the bare minimal that would be wanted.
            Hide
            bawjaws David Scotson added a comment -

            Just as an example of what I was talking about, the following CSS can be pasted into the Custom CSS box in the Clean theme, and it'll give the the basic look of the Cerulean Bootswatch (a blue header with white text and a slighly lighter blue for links/hover states) see the full thing here: http://bootswatch.com/2/cerulean/ . All the below bits were cut'n'pasted from http://bootswatch.com/2/cerulean/bootstrap.css and had (most) non-color related rules deleted.

            @import url(//fonts.googleapis.com/css?family=Telex);
            body {
              font-size: 14px;
              line-height: 20px;
            }
             
            a {
              color: #2fa4e7;
            }
             
            a:hover,
            a:focus {
              color: #157ab5;
            }
             
            .navbar-inner {
              background-color: #45aeea;
              background-image: linear-gradient(to bottom, #54b4eb, #2fa4e7);
              border: 1px solid #1990d5;
            }
             
            .navbar .brand {
              color: #ffffff;
              text-shadow: 0 1px 0 #54b4eb;
            }
             
            .navbar-text {
              color: #f5f5f5;
            }
             
            .navbar-link {
              color: #ffffff;
            }
             
            .navbar-link:hover,
            .navbar-link:focus {
              color: #ffffff;
            }
             
            .navbar .divider-vertical {
              border-right-color: #54b4eb;
              border-left-color: #2fa4e7;
            }
             
            .navbar .logininfo a {
            color: #fff;
            }
            .dropdown-menu > li > a:hover,
            .dropdown-menu > li > a:focus,
            .dropdown-submenu:hover > a,
            .dropdown-submenu:focus > a {
              color: #ffffff;
              background-color: #27a0e5;
              background-image: linear-gradient(to bottom, #2fa4e7, #1a99e2);
            }
             
            .dropdown-menu > .active > a,
            .dropdown-menu > .active > a:hover,
            .dropdown-menu > .active > a:focus {
              color: #ffffff;
              background-color: #27a0e5;
              background-image: linear-gradient(to bottom, #2fa4e7, #1a99e2);
            }
             
            .navbar .nav li.dropdown > a:hover .caret,
            .navbar .nav li.dropdown > a:focus .caret {
              border-top-color: #ffffff;
              border-bottom-color: #ffffff;
            }
             
            .navbar .nav li.dropdown.open > .dropdown-toggle,
            .navbar .nav li.dropdown.active > .dropdown-toggle,
            .navbar .nav li.dropdown.open.active > .dropdown-toggle {
              color: #ffffff;
              background-color: #1684c2;
            }
             
            .navbar .nav li.dropdown > .dropdown-toggle .caret {
              border-top-color: #ffffff;
              border-bottom-color: #ffffff;
            }
             
            .navbar .nav li.dropdown.open > .dropdown-toggle .caret,
            .navbar .nav li.dropdown.active > .dropdown-toggle .caret,
            .navbar .nav li.dropdown.open.active > .dropdown-toggle .caret {
              border-top-color: #ffffff;
              border-bottom-color: #ffffff;
            }
             
            .navbar .nav > li > a {
              padding: 15px 15px 15px;
              color: #ffffff;
              text-shadow: 0 1px 0 #54b4eb;
            }
             
            .navbar .nav > li > a:focus,
            .navbar .nav > li > a:hover {
              color: #ffffff;
              background-color: #1684c2;
            }
             
            .navbar .nav > .active > a,
            .navbar .nav > .active > a:hover,
            .navbar .nav > .active > a:focus {
              color: #ffffff;
              background-color: #1684c2;
                      box-shadow: inset 0 3px 8px rgba(0, 0, 0, 0.125);
            }
             
            h1,
            h2,
            h3,
            h4,
            h5,
            h6 {
               font-family: 'Telex', sans-serif;
              line-height: 20px;
              color: #317eac;
            }

            So a version of that CSS with the hardcoded #colors replaced with 8 or so color variables (navbar-background, navbar-gradient-start, navbar-gradient-end, navbar-text, link-text and so-on, also a Google Font, which aren't colors but are a easy way to 'brand' a site) that are replaced with the standard theme variable methods would get you pretty far I think.

            You can continue on towards replacing every color in Bootstrap in the same manner, but I think you hit diminishing returns very quickly and get most of the desired result with very little effort.

            Show
            bawjaws David Scotson added a comment - Just as an example of what I was talking about, the following CSS can be pasted into the Custom CSS box in the Clean theme, and it'll give the the basic look of the Cerulean Bootswatch (a blue header with white text and a slighly lighter blue for links/hover states) see the full thing here: http://bootswatch.com/2/cerulean/ . All the below bits were cut'n'pasted from http://bootswatch.com/2/cerulean/bootstrap.css and had (most) non-color related rules deleted. @import url(//fonts.googleapis.com/css?family=Telex); body { font-size: 14px; line-height: 20px; }   a { color: #2fa4e7; }   a:hover, a:focus { color: #157ab5; }   .navbar-inner { background-color: #45aeea; background-image: linear-gradient(to bottom, #54b4eb, #2fa4e7); border: 1px solid #1990d5; }   .navbar .brand { color: #ffffff; text-shadow: 0 1px 0 #54b4eb; }   .navbar-text { color: #f5f5f5; }   .navbar-link { color: #ffffff; }   .navbar-link:hover, .navbar-link:focus { color: #ffffff; }   .navbar .divider-vertical { border-right-color: #54b4eb; border-left-color: #2fa4e7; }   .navbar .logininfo a { color: #fff; } .dropdown-menu > li > a:hover, .dropdown-menu > li > a:focus, .dropdown-submenu:hover > a, .dropdown-submenu:focus > a { color: #ffffff; background-color: #27a0e5; background-image: linear-gradient(to bottom, #2fa4e7, #1a99e2); }   .dropdown-menu > .active > a, .dropdown-menu > .active > a:hover, .dropdown-menu > .active > a:focus { color: #ffffff; background-color: #27a0e5; background-image: linear-gradient(to bottom, #2fa4e7, #1a99e2); }   .navbar .nav li.dropdown > a:hover .caret, .navbar .nav li.dropdown > a:focus .caret { border-top-color: #ffffff; border-bottom-color: #ffffff; }   .navbar .nav li.dropdown.open > .dropdown-toggle, .navbar .nav li.dropdown.active > .dropdown-toggle, .navbar .nav li.dropdown.open.active > .dropdown-toggle { color: #ffffff; background-color: #1684c2; }   .navbar .nav li.dropdown > .dropdown-toggle .caret { border-top-color: #ffffff; border-bottom-color: #ffffff; }   .navbar .nav li.dropdown.open > .dropdown-toggle .caret, .navbar .nav li.dropdown.active > .dropdown-toggle .caret, .navbar .nav li.dropdown.open.active > .dropdown-toggle .caret { border-top-color: #ffffff; border-bottom-color: #ffffff; }   .navbar .nav > li > a { padding: 15px 15px 15px; color: #ffffff; text-shadow: 0 1px 0 #54b4eb; }   .navbar .nav > li > a:focus, .navbar .nav > li > a:hover { color: #ffffff; background-color: #1684c2; }   .navbar .nav > .active > a, .navbar .nav > .active > a:hover, .navbar .nav > .active > a:focus { color: #ffffff; background-color: #1684c2; box-shadow: inset 0 3px 8px rgba(0, 0, 0, 0.125); }   h1, h2, h3, h4, h5, h6 { font-family: 'Telex', sans-serif; line-height: 20px; color: #317eac; } So a version of that CSS with the hardcoded #colors replaced with 8 or so color variables (navbar-background, navbar-gradient-start, navbar-gradient-end, navbar-text, link-text and so-on, also a Google Font, which aren't colors but are a easy way to 'brand' a site) that are replaced with the standard theme variable methods would get you pretty far I think. You can continue on towards replacing every color in Bootstrap in the same manner, but I think you hit diminishing returns very quickly and get most of the desired result with very little effort.
            Hide
            lazydaisy Mary Evans added a comment -

            Minimal sounds good as it would, in effect, be Bootstrap as it is now, plain and simple. Would you like me to prepare it ready for 2.7 launch?

            I've deliberately not commented up to now, as the development work with LESS and PHP is way over my head, but I do see the benefits it could offer.

            Show
            lazydaisy Mary Evans added a comment - Minimal sounds good as it would, in effect, be Bootstrap as it is now, plain and simple. Would you like me to prepare it ready for 2.7 launch? I've deliberately not commented up to now, as the development work with LESS and PHP is way over my head, but I do see the benefits it could offer.
            Hide
            lazydaisy Mary Evans added a comment -

            @Martin,

            I think 'Custom' as a name for this PHP/LESS theme would be a good choice.

            Show
            lazydaisy Mary Evans added a comment - @Martin, I think 'Custom' as a name for this PHP/LESS theme would be a good choice.
            Hide
            lazydaisy Mary Evans added a comment -

            Moodle Themes in a Nutshell:

            Minimal = Bootstrap (plain and simple)

            Clean = Minimal + a few settings (clean and easy)

            Custom = Clean + LESS (highly customisable)

            Show
            lazydaisy Mary Evans added a comment - Moodle Themes in a Nutshell: Minimal = Bootstrap (plain and simple) Clean = Minimal + a few settings (clean and easy) Custom = Clean + LESS (highly customisable)
            Hide
            dougiamas Martin Dougiamas added a comment -

            David I guess that could be called a "Moodle Clean Swatch" and we could certainly have a little database of these somewhere (even in the theme itself, under a menu) as a kind of Preset. But it's still far too complicated to expect a novice to be editing. We still need color pickers and simple settings in at least one theme.

            Show
            dougiamas Martin Dougiamas added a comment - David I guess that could be called a "Moodle Clean Swatch" and we could certainly have a little database of these somewhere (even in the theme itself, under a menu) as a kind of Preset. But it's still far too complicated to expect a novice to be editing. We still need color pickers and simple settings in at least one theme.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Mary, if people are happy with Clean as it is for development and other basic purposes then we could get away with just two I think.

            It would be either:

            1. Clean (as is) + Custom (Clean + LESS)
            2. Minimal (new) + Clean (as is + LESS)

            Show
            dougiamas Martin Dougiamas added a comment - Mary, if people are happy with Clean as it is for development and other basic purposes then we could get away with just two I think. It would be either: 1. Clean (as is) + Custom (Clean + LESS) 2. Minimal (new) + Clean (as is + LESS)
            Hide
            bawjaws David Scotson added a comment -

            I'd suggest limiting the scope of this new theme to a very basic task: a nice, simple theme that matches one (or maybe two) of your school's brand colors with minimal effort. Given Bootstrap as a starting point this basically means choosing the color of the navbar and the text within it.

            With that in mind perhaps "Custom" isn't a great name choice as it suggests anything is possible, e.g. Mary just described it as "highly customisable" based on it's name, and it sounds like that's what Martin and Fred envision for it too. That sounds like it could turn into a massive undertaking (assuming it hasn't already). For those that want/need something highly customisable, it's better for everyone if they just dig into the theme developer tools directly.

            So why not call it Branded, or ColorSelect, or something else that focusses on what end users actually want from this, a way to match Moodle with their current logo colors (and maybe typeface). Doing that well would provide real value to users and seems very achievable, even if the tech involved isn't particularly exciting.

            Show
            bawjaws David Scotson added a comment - I'd suggest limiting the scope of this new theme to a very basic task: a nice, simple theme that matches one (or maybe two) of your school's brand colors with minimal effort. Given Bootstrap as a starting point this basically means choosing the color of the navbar and the text within it. With that in mind perhaps "Custom" isn't a great name choice as it suggests anything is possible, e.g. Mary just described it as "highly customisable" based on it's name, and it sounds like that's what Martin and Fred envision for it too. That sounds like it could turn into a massive undertaking (assuming it hasn't already). For those that want/need something highly customisable, it's better for everyone if they just dig into the theme developer tools directly. So why not call it Branded, or ColorSelect, or something else that focusses on what end users actually want from this, a way to match Moodle with their current logo colors (and maybe typeface). Doing that well would provide real value to users and seems very achievable, even if the tech involved isn't particularly exciting.
            Hide
            bawjaws David Scotson added a comment -

            Hi Martin,

            I'm not suggesting end users do the editing, I'm suggesting that core provides that CSS behind the scenes and offers a color picker interface to choose what colors it provides. A brutully simple implementation would perhaps require 8 colors to be chosen, a bit of cleverness in PHP could reduce that to 1 or 2 colors needing to be select (with the other darker/lighter shades of the same color generated by code).

            I provided the CSS in it's entirety to let people experience that, right now, with a cut'n'paste of a small amount of CSS it's possible to radically alter the appearance of Clean theme in a way that end users actually want. And that could be built into any Bootstrap-based theme today, with no LESS involved, just standard, boring Moodle theme techniques.

            Show
            bawjaws David Scotson added a comment - Hi Martin, I'm not suggesting end users do the editing, I'm suggesting that core provides that CSS behind the scenes and offers a color picker interface to choose what colors it provides. A brutully simple implementation would perhaps require 8 colors to be chosen, a bit of cleverness in PHP could reduce that to 1 or 2 colors needing to be select (with the other darker/lighter shades of the same color generated by code). I provided the CSS in it's entirety to let people experience that, right now, with a cut'n'paste of a small amount of CSS it's possible to radically alter the appearance of Clean theme in a way that end users actually want. And that could be built into any Bootstrap-based theme today, with no LESS involved, just standard, boring Moodle theme techniques.
            Hide
            lazydaisy Mary Evans added a comment -

            We all have our own visions...describing them as we see them in our minds eye.

            If we carry on like this we will end up with a 'camel' with more 'humps' than it can carry!

            Show
            lazydaisy Mary Evans added a comment - We all have our own visions...describing them as we see them in our minds eye. If we carry on like this we will end up with a 'camel' with more 'humps' than it can carry!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I must confess that, still, I don't get the point about the compiler at all, in fact I think we are already using [[pix|font]] placeholders in bootstrapbase that later are processed by theme_config->post_process().

            I fact, I've quickly tried adding this less file to clean:

            @mainColor:      ~"[[setting:maincolor]]";
            @happyEmoticon:  ~"[[pix:moodle|s/smiley.gif]]";
            @backgoundImage: ~"[[pix:theme_bootstrapbase|screenshot.jpg]]";
            @textSize:       ~"[[setting:textsize]]";
             
            #page-theme-clean-test {
                font-size:  @textSize;
                color:      @mainColor;
                background: url(@backgoundImage);
                p {
                    background: url(@happyEmoticon);
                }
                i {
                    background: url(@happyEmoticon);
                }
            }

            that, once recess-compiled have become:

            #page-theme-clean-test {
              font-size: [[setting:textsize]];
              color: [[setting:maincolor]];
              background: url([[pix:theme_bootstrapbase|screenshot.jpg]]);
            }
             
            #page-theme-clean-test p {
              background: url([[pix:moodle|s/smiley.gif]]);
            }
             
            #page-theme-clean-test i {
              background: url([[pix:moodle|s/smiley.gif]]);
            }

            and once served is:

            #page-theme-clean-test {
                font-size: [[setting: textsize]];
                color: [[setting: maincolor]];
                background: url(/~stronk7/git/theme/image.php/clean/theme_bootstrapbase/1394642498/screenshot.jpg)
            }
             
            #page-theme-clean-test
            p {
                background: url(/~stronk7/git/theme/image.php/clean/core/1394642498/s/smiley.gif)
            }
             
            #page-theme-clean-test
            i {
                background: url(/~stronk7/git/theme/image.php/clean/core/1394642498/s/smiley.gif)
            }

            Note that [[setting]] occurrences are not processed because neither theme_config neither clean->csspostprocess support them, but other themes out there (base/canvas based) are replacing [[settings]] perfectly.

            So I don't find a reason for bringing any compiler into core, when placeholder replacement is 100% possible for any theme already. I only can imagine more complex situations, where other less features are required (say calculations against those placeholders or something like that), but for plain and raw replacements... we already support them.

            Perhaps it would be interesting to extend support for [[setting]] placeholders moodle-wide and bump, done.

            Note I've not tested anything, just created the example above to see how transformations were happening, and seems to happen as expected, providing out-of-the-box replacement without a problem.

            Also, if there are (some of those complex) reasons to add the compiler, I agree it's better to confine it within a theme, but only if there are reasons.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I must confess that, still, I don't get the point about the compiler at all, in fact I think we are already using [ [pix|font] ] placeholders in bootstrapbase that later are processed by theme_config->post_process(). I fact, I've quickly tried adding this less file to clean: @mainColor: ~"[[setting:maincolor]]"; @happyEmoticon: ~"[[pix:moodle|s/smiley.gif]]"; @backgoundImage: ~"[[pix:theme_bootstrapbase|screenshot.jpg]]"; @textSize: ~"[[setting:textsize]]";   #page-theme-clean-test { font-size: @textSize; color: @mainColor; background: url(@backgoundImage); p { background: url(@happyEmoticon); } i { background: url(@happyEmoticon); } } that, once recess-compiled have become: #page-theme-clean-test { font-size: [[setting:textsize]]; color: [[setting:maincolor]]; background: url([[pix:theme_bootstrapbase|screenshot.jpg]]); }   #page-theme-clean-test p { background: url([[pix:moodle|s/smiley.gif]]); }   #page-theme-clean-test i { background: url([[pix:moodle|s/smiley.gif]]); } and once served is: #page-theme-clean-test { font-size: [[setting: textsize]]; color: [[setting: maincolor]]; background: url(/~stronk7/git/theme/image.php/clean/theme_bootstrapbase/1394642498/screenshot.jpg) }   #page-theme-clean-test p { background: url(/~stronk7/git/theme/image.php/clean/core/1394642498/s/smiley.gif) }   #page-theme-clean-test i { background: url(/~stronk7/git/theme/image.php/clean/core/1394642498/s/smiley.gif) } Note that [ [setting] ] occurrences are not processed because neither theme_config neither clean->csspostprocess support them, but other themes out there (base/canvas based) are replacing [ [settings] ] perfectly. So I don't find a reason for bringing any compiler into core, when placeholder replacement is 100% possible for any theme already. I only can imagine more complex situations, where other less features are required (say calculations against those placeholders or something like that), but for plain and raw replacements... we already support them. Perhaps it would be interesting to extend support for [ [setting] ] placeholders moodle-wide and bump, done. Note I've not tested anything, just created the example above to see how transformations were happening, and seems to happen as expected, providing out-of-the-box replacement without a problem. Also, if there are (some of those complex) reasons to add the compiler, I agree it's better to confine it within a theme, but only if there are reasons. Ciao
            Hide
            lazydaisy Mary Evans added a comment -

            Are you saying Clean does not use [[setting]] placeholders? Or am I misreading what you have said...because Clean theme does use settings, here's the CSS.

            /* Custom CSS
            -------------------------*/
            a.logo {
                background: url([[setting:logo]]) no-repeat 0 0;
                display: block;
                float: left;
                height: 75px;
                margin: 0;
                padding: 0;
                width: 100%;
            }
             
            .dir-rtl a.logo {
                background: url([[setting:logo]]) no-repeat 100% 0;
                display: block;
                float: right;
            }
             
            /* Custom CSS Settings
            -------------------------*/
            [[setting:customcss]]
             

            Show
            lazydaisy Mary Evans added a comment - Are you saying Clean does not use [ [setting] ] placeholders? Or am I misreading what you have said...because Clean theme does use settings, here's the CSS. /* Custom CSS -------------------------*/ a.logo { background: url([[setting:logo]]) no-repeat 0 0; display: block; float: left; height: 75px; margin: 0; padding: 0; width: 100%; }   .dir-rtl a.logo { background: url([[setting:logo]]) no-repeat 100% 0; display: block; float: right; }   /* Custom CSS Settings -------------------------*/ [[setting:customcss]]  
            Hide
            samhemelryk Sam Hemelryk added a comment - - edited

            I've just got to the bottom of this, I'm still processing things but just to offer some aid:
            Eloy, the need for the compiler comes from Bootstraps advanced functionality, the darken method for one example:

            # bootstrap/variables.less contains:
            @linkColor: #08c;
            @linkColorHover: darken(@linkColor, 15%);
            

            That darken is the reason we can't do use a double substitution (sorry I didn't factor this into my thinking while discussing it with you yesterday).
            If we defined @linkColor: ~"maincolor"; the less compiler would complain (likely buff up and we'd get no styles) because it would not be able to darken "maincolor".
            (just fyi less is not a linear parser, variable substitution + function execution occurs last)

            Mary, I'm guessing Eloy was just saying that the variables he's defined weren't replaced. This would obviously be because variable substitution is just string replacement, and bootstrapbase/clean are not replacing the settings he's used.

            So - after I've had a bit more time to roll this around in my mind I will be back to share my thoughts.

            Show
            samhemelryk Sam Hemelryk added a comment - - edited I've just got to the bottom of this, I'm still processing things but just to offer some aid: Eloy, the need for the compiler comes from Bootstraps advanced functionality, the darken method for one example: # bootstrap/variables.less contains: @linkColor: #08c; @linkColorHover: darken(@linkColor, 15%); That darken is the reason we can't do use a double substitution (sorry I didn't factor this into my thinking while discussing it with you yesterday). If we defined @linkColor: ~"maincolor"; the less compiler would complain (likely buff up and we'd get no styles) because it would not be able to darken "maincolor". (just fyi less is not a linear parser, variable substitution + function execution occurs last) Mary, I'm guessing Eloy was just saying that the variables he's defined weren't replaced. This would obviously be because variable substitution is just string replacement, and bootstrapbase/clean are not replacing the settings he's used. So - after I've had a bit more time to roll this around in my mind I will be back to share my thoughts.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            First up - I've briefly shared this issue in the themes forum https://moodle.org/mod/forum/discuss.php?d=256166 I bet it will be of interest to others not already participating here.

            Having thought about this for a while here's my opinion - in a nutshell - I like the separate theme ideas, great separation of ideas, clear points about objectives and maintenance, +1 from me for that.
            Including the less compiler in theme at first threw off, I was wondering how that would work. But now that I think about it, having it in the theme, where it is required to support only what the theme requires sounds like a very good idea to me.
            That way we're not bound to end up in situations where people want later versions back ported in order to support new/alternative frameworks they wish to use in their themes.
            Duplication between themes is perhaps a downside, but really in the scheme of things its not a big library, and this one core theme will serve as an example of how to do it.
            I have one question, can someone still extend the theme and make use of the less compiler should they wish to (and of course someone will)?
            Perhaps it'll be easiest to see that once the code has been written.

            Naming wise I don't like custom, that to me just sounds like a nothing word put there to confuse and as someone mentioned it won't necessarily ring true to the theme.
            Martin purposed "2. Minimal (new) + Clean (as is + LESS)" that gets a +1 from me.

            Now rebuttal
            Strict CSS - I still have concerns here. When I was playing with this, with only the compiler on my branch (so without the grading fix) I was not getting ANY CSS at all for the page, I was getting a completely un-styled page.
            If there was meant to be a fallback system in place to deliver old skool CSS and I just wasn't seeing it then fine - but if we are happy with delivering no CSS when there is a single CSS bug I think we are horribly wrong.
            If the fallback is there I am fine, consistent output should be considered still of course.

            On the point of sourcemaps Fred you missed my point. Without them finding CSS bugs is harder in 2.7 than it was in 2.6 as the source file will now be obscured.
            Granted most people don't know about them yet, but after this change lands they will want to, because life got harder. Nested statements, functions, media queries, and variables, debugging is getting harder.
            Martin mentioned that not supporting less in plugins will allow us to deliver split files, if that's the case I am happy, otherwise I would still like to see at least an issue to add support for sourcemaps.

            Slowness, Martin you mentioned that its not uncommon to already have 10s page load times which is very true. This however adds 10s to the page rendering time as we wait for the CSS to be delivered.
            Again, unfortunate, not terrible, not a blocker, just unfortunate. Having a separate theme for this I think voids this as a concern really as it will only affect those designing with that theme, and as that theme is customisable it won't be common to want to customise it.

            Yay for plugins not have less files that are compiled by PHP, that cuts out what I am sure would have been a nightmare.
            Just to clarify, this doesn't mean you cant have a less file in your plugin, it just means that you (the developer) must compile it yourself before releasing your plugin; exactly like you would presently if you had used one.
            As everyone is aware having a good frontend structure available within Moodle would fix this. Please someone love the element library.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - First up - I've briefly shared this issue in the themes forum https://moodle.org/mod/forum/discuss.php?d=256166 I bet it will be of interest to others not already participating here. Having thought about this for a while here's my opinion - in a nutshell - I like the separate theme ideas, great separation of ideas, clear points about objectives and maintenance, +1 from me for that. Including the less compiler in theme at first threw off, I was wondering how that would work. But now that I think about it, having it in the theme, where it is required to support only what the theme requires sounds like a very good idea to me. That way we're not bound to end up in situations where people want later versions back ported in order to support new/alternative frameworks they wish to use in their themes. Duplication between themes is perhaps a downside, but really in the scheme of things its not a big library, and this one core theme will serve as an example of how to do it. I have one question, can someone still extend the theme and make use of the less compiler should they wish to (and of course someone will)? Perhaps it'll be easiest to see that once the code has been written. Naming wise I don't like custom, that to me just sounds like a nothing word put there to confuse and as someone mentioned it won't necessarily ring true to the theme. Martin purposed "2. Minimal (new) + Clean (as is + LESS)" that gets a +1 from me. Now rebuttal Strict CSS - I still have concerns here. When I was playing with this, with only the compiler on my branch (so without the grading fix) I was not getting ANY CSS at all for the page, I was getting a completely un-styled page. If there was meant to be a fallback system in place to deliver old skool CSS and I just wasn't seeing it then fine - but if we are happy with delivering no CSS when there is a single CSS bug I think we are horribly wrong. If the fallback is there I am fine, consistent output should be considered still of course. On the point of sourcemaps Fred you missed my point. Without them finding CSS bugs is harder in 2.7 than it was in 2.6 as the source file will now be obscured. Granted most people don't know about them yet, but after this change lands they will want to, because life got harder. Nested statements, functions, media queries, and variables, debugging is getting harder. Martin mentioned that not supporting less in plugins will allow us to deliver split files, if that's the case I am happy, otherwise I would still like to see at least an issue to add support for sourcemaps. Slowness, Martin you mentioned that its not uncommon to already have 10s page load times which is very true. This however adds 10s to the page rendering time as we wait for the CSS to be delivered. Again, unfortunate, not terrible, not a blocker, just unfortunate. Having a separate theme for this I think voids this as a concern really as it will only affect those designing with that theme, and as that theme is customisable it won't be common to want to customise it. Yay for plugins not have less files that are compiled by PHP, that cuts out what I am sure would have been a nightmare. Just to clarify, this doesn't mean you cant have a less file in your plugin, it just means that you (the developer) must compile it yourself before releasing your plugin; exactly like you would presently if you had used one. As everyone is aware having a good frontend structure available within Moodle would fix this. Please someone love the element library. Cheers Sam
            Hide
            gb2048 Gareth J Barnard added a comment -

            The new theme name does not have to be 'Custom', I just think that you will save yourselves a shed load of work updating all the code / comments and associated documentation if you keep 'Clean' as is bar reducing some of the code and have a new name for the theme with the wizz bang LESS. Could easily be called 'Bespoke', 'Tinker', 'Fiddle'....

            Show
            gb2048 Gareth J Barnard added a comment - The new theme name does not have to be 'Custom', I just think that you will save yourselves a shed load of work updating all the code / comments and associated documentation if you keep 'Clean' as is bar reducing some of the code and have a new name for the theme with the wizz bang LESS. Could easily be called 'Bespoke', 'Tinker', 'Fiddle'....
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Mary Evans, yes what I was trying to say is that, perhaps we should generalize settings support. Right now every theme has its own processor, many of them implementing support for different [[settings]]. IMO it would be far better having it in core (the same we do for both [[pix]] and [[font]]). And yes, I saw clean processor had support for [[setting:logo]], but for nothing else. That's the point.

            Sam Hemelryk, yes those were the type of "complex" reasons (less features) I was imagining would still require an online compilation. No matter of that, again, I'd propose to provide support for simple settings for all themes. Then designers can decide where they base their theme (without or with compiler).

            About how the theme names and/or hierarchies... I've nothing to say. Just, whatever it's finally decided, document it clearly.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Mary Evans , yes what I was trying to say is that, perhaps we should generalize settings support. Right now every theme has its own processor, many of them implementing support for different [ [settings] ]. IMO it would be far better having it in core (the same we do for both [ [pix] ] and [ [font] ]). And yes, I saw clean processor had support for [ [setting:logo] ], but for nothing else. That's the point. Sam Hemelryk , yes those were the type of "complex" reasons (less features) I was imagining would still require an online compilation. No matter of that, again, I'd propose to provide support for simple settings for all themes. Then designers can decide where they base their theme (without or with compiler). About how the theme names and/or hierarchies... I've nothing to say. Just, whatever it's finally decided, document it clearly. Ciao
            Hide
            fred Frédéric Massart added a comment -

            In regards with Sam's comment, I am not sure what happened when the compile failed, but in theory it should have fallen back on bootstrapbase CSS files. So if we go for minimal + clean, if LESS fails, Clean would look 100% like minimal. Also, I raised MDL-44957 for source maps.

            Just some comments to consider:

            1. +1 Remove .less from plugins, no problem.
            2. +1 for Minimal (new) + Clean (as is + LESS)
            3. -1 for the LESS library in the theme
              • It would create problems when you duplicate the theme, designers will not extend it but duplicate it, that could cause some random issues
              • The LESS logic is in outputlib.php, not in the theme itself. Hacking the theme to get the LESS compilation in it does not seem right... the only way I can think of for now is by having post_process() injecting 4000 lines of CSS.
              • When you compile LESS you ignore some CSS of the parents, except when the LESS fails... reproducing this in the theme itself makes it really hacky!

            Of course we can entirely revise the way I worked around the LESS issue and make the theme able to return raw CSS, but is that right?

            Show
            fred Frédéric Massart added a comment - In regards with Sam's comment, I am not sure what happened when the compile failed, but in theory it should have fallen back on bootstrapbase CSS files. So if we go for minimal + clean, if LESS fails, Clean would look 100% like minimal. Also, I raised MDL-44957 for source maps. Just some comments to consider: +1 Remove .less from plugins, no problem. +1 for Minimal (new) + Clean (as is + LESS) -1 for the LESS library in the theme It would create problems when you duplicate the theme, designers will not extend it but duplicate it, that could cause some random issues The LESS logic is in outputlib.php, not in the theme itself. Hacking the theme to get the LESS compilation in it does not seem right... the only way I can think of for now is by having post_process() injecting 4000 lines of CSS. When you compile LESS you ignore some CSS of the parents, except when the LESS fails... reproducing this in the theme itself makes it really hacky! Of course we can entirely revise the way I worked around the LESS issue and make the theme able to return raw CSS, but is that right?
            Hide
            bawjaws David Scotson added a comment -

            Hi Fred,

            When I did less compilation in a theme I used post_process() to insert the generated CSS. I didn't see it that much conceptually different from generating the CSS via the standard javascript methods and then presenting Moodle with a single minified CSS file in the style folder, as currently done in Bootstrapbase.

            Show
            bawjaws David Scotson added a comment - Hi Fred, When I did less compilation in a theme I used post_process() to insert the generated CSS. I didn't see it that much conceptually different from generating the CSS via the standard javascript methods and then presenting Moodle with a single minified CSS file in the style folder, as currently done in Bootstrapbase.
            Hide
            bushido Mark Nielsen added a comment -

            Frédéric Massart:

            -1 for the LESS library in the theme

            It does seem unintuitive at first, libraries should be placed somewhere so it can be shared. But I think if you give a theme the option of "hey, give me the CSS you want me to use" OR "Hey, give me the extra CSS you want me to use" then you open up a lot of possibilities to theme developers. They could even use other technologies like SASS or different versions (probably newer) of compilers, etc. Or other custom processing. This would of course fallback to standard CSS processing though if the theme didn't want to do anything custom.

            David Scotson: Regarding post_process(), this looked promising to me until I saw how debug styles uses it. Seems like your extra CSS would be appended to X number of debug style sheets. Not sure if this is good or bad, but I took it as bad. Petr might have changed this in MDL-44358 though.

            My 2 cents, cheers!

            Show
            bushido Mark Nielsen added a comment - Frédéric Massart : -1 for the LESS library in the theme It does seem unintuitive at first, libraries should be placed somewhere so it can be shared. But I think if you give a theme the option of "hey, give me the CSS you want me to use" OR "Hey, give me the extra CSS you want me to use" then you open up a lot of possibilities to theme developers. They could even use other technologies like SASS or different versions (probably newer) of compilers, etc. Or other custom processing. This would of course fallback to standard CSS processing though if the theme didn't want to do anything custom. David Scotson : Regarding post_process(), this looked promising to me until I saw how debug styles uses it. Seems like your extra CSS would be appended to X number of debug style sheets. Not sure if this is good or bad, but I took it as bad. Petr might have changed this in MDL-44358 though. My 2 cents, cheers!
            Hide
            bawjaws David Scotson added a comment -

            Hi Mark,

            the way I did it was to have a marker in the theme CSS file, let's say it was [[less_output_goes_here]], and then the output of the less compilation looks for and replaces that tag, similar to the theme settings stuff. That means the core CSS passes through untouched.

            But an API that just let you inject CSS from you PHP without having to do that would be nice too.

            Show
            bawjaws David Scotson added a comment - Hi Mark, the way I did it was to have a marker in the theme CSS file, let's say it was [ [less_output_goes_here] ], and then the output of the less compilation looks for and replaces that tag, similar to the theme settings stuff. That means the core CSS passes through untouched. But an API that just let you inject CSS from you PHP without having to do that would be nice too.
            Hide
            bushido Mark Nielsen added a comment -

            Ah, very clever!

            Show
            bushido Mark Nielsen added a comment - Ah, very clever!
            Hide
            fred Frédéric Massart added a comment - - edited

            Actually you are right David, that is possible. But that solution does not allow for any fallback. Or it could, but that would be more and more hacky.

            Show
            fred Frédéric Massart added a comment - - edited Actually you are right David, that is possible. But that solution does not allow for any fallback. Or it could, but that would be more and more hacky.
            Hide
            bawjaws David Scotson added a comment -

            Hi Fred,

            bear in mind that I don't think compiling the entire theme from less on the fly is a good idea in the first place, so I see the need for fallbacks for when it all goes wrong as part of the problem.

            I did it last year as an experiment to see if it had any benefits and mostly I found it didn't, so abandoned the idea (one key negative was that Bootstrap pushes the very limits of less.js, and the PHP version broke on nearly every minor point update to Bootstrap, though maybe the new PHP library will avoid that). Slowness was another big thing (and that was using the older, faster library). For actually developing Bootstrap based themes I find the grunt based workflows (for which you don't even need Theme Developer mode switched on!) far more civilized if you have access to the server and node.js.

            For those that want to add a splash of colour to their theme via the web interface the less compilation in PHP is technically neat, but I don't see many benefits over the standard theme variable substitution for any use case that's been mentioned so far. (It is after all a system flexible enough to allow you to completely generate your theme with less in PHP and insert the CSS output so it can do quite a lot of other stuff too). But if some live less compilation in PHP was required by some future need, I'd recommend limiting what you compile as much as possible. Although the standard less workflow ends with a single minified CSS file, it is possible to generate multiple smaller files that work together (e.g. the responsive stuff in Bootstrap 2.3, or some of the third party RTL versions that are generated and then served as separate CSS files). This avoids the whole theme disappearing due to a single typo.

            Show
            bawjaws David Scotson added a comment - Hi Fred, bear in mind that I don't think compiling the entire theme from less on the fly is a good idea in the first place, so I see the need for fallbacks for when it all goes wrong as part of the problem. I did it last year as an experiment to see if it had any benefits and mostly I found it didn't, so abandoned the idea (one key negative was that Bootstrap pushes the very limits of less.js, and the PHP version broke on nearly every minor point update to Bootstrap, though maybe the new PHP library will avoid that). Slowness was another big thing (and that was using the older, faster library). For actually developing Bootstrap based themes I find the grunt based workflows (for which you don't even need Theme Developer mode switched on!) far more civilized if you have access to the server and node.js. For those that want to add a splash of colour to their theme via the web interface the less compilation in PHP is technically neat, but I don't see many benefits over the standard theme variable substitution for any use case that's been mentioned so far. (It is after all a system flexible enough to allow you to completely generate your theme with less in PHP and insert the CSS output so it can do quite a lot of other stuff too). But if some live less compilation in PHP was required by some future need, I'd recommend limiting what you compile as much as possible. Although the standard less workflow ends with a single minified CSS file, it is possible to generate multiple smaller files that work together (e.g. the responsive stuff in Bootstrap 2.3, or some of the third party RTL versions that are generated and then served as separate CSS files). This avoids the whole theme disappearing due to a single typo.
            Hide
            fred Frédéric Massart added a comment -

            Sending a new version for peer review.

            1. I removed the LESS option for plugins
            2. Thus the LESS file is standalone and does not include everything from everywhere
            3. There is no more callback when the compiling fails, this can be added later
            4. The library is included in core, and the logic is in outputlib. I can work on a dirty theme that uses post_process but I don't think it's ideal.
            5. The new theme has been created, it is an exact copy of Clean but will get more settings in the related issue. Martin Dougiamas finally preferred keeping Clean as it is rather than adding or removing settings.
            6. The new theme is called More, because it is using Less (ah ah)... but I'm sure we'll have to discuss it > http://martinfowler.com/bliki/TwoHardThings.html
            7. I could/should possibly squash my commits

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Sending a new version for peer review. I removed the LESS option for plugins Thus the LESS file is standalone and does not include everything from everywhere There is no more callback when the compiling fails, this can be added later The library is included in core, and the logic is in outputlib. I can work on a dirty theme that uses post_process but I don't think it's ideal. The new theme has been created, it is an exact copy of Clean but will get more settings in the related issue. Martin Dougiamas finally preferred keeping Clean as it is rather than adding or removing settings. The new theme is called More , because it is using Less (ah ah)... but I'm sure we'll have to discuss it > http://martinfowler.com/bliki/TwoHardThings.html I could/should possibly squash my commits Cheers, Fred
            Hide
            fred Frédéric Massart added a comment -

            (I've reset the fields Peer reviewer and Integrator).

            Show
            fred Frédéric Massart added a comment - (I've reset the fields Peer reviewer and Integrator).
            Hide
            cibot CiBoT added a comment -

            Results for MDL-44357

            • Remote repository: git://github.com/FMCorz/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-44357 Remote repository: git://github.com/FMCorz/moodle.git Remote branch MDL-44357 -master-noplugin to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2323 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2323/artifact/work/smurf.html
            Hide
            damyon Damyon Wiese added a comment -

            Still looking - just some comments from reading through the code.

            1 - nice job - the code looks great and almost perfect.
            2 - how does this work if I define a theme that inherits from more - it seems the css from the less file in the parent will not be sent - is this intended? I would assume that if I inherit and do nothing else, my theme should look like the parent.
            3 - It looks like you could release the same lock twice in styles.php - setting the lock to false/null after releasing it the first time would cover this.

            Show
            damyon Damyon Wiese added a comment - Still looking - just some comments from reading through the code. 1 - nice job - the code looks great and almost perfect. 2 - how does this work if I define a theme that inherits from more - it seems the css from the less file in the parent will not be sent - is this intended? I would assume that if I inherit and do nothing else, my theme should look like the parent. 3 - It looks like you could release the same lock twice in styles.php - setting the lock to false/null after releasing it the first time would cover this.
            Hide
            damyon Damyon Wiese added a comment -

            I tested 2 and it works as expected - so good job.

            Also - 4. Can we have an updated screenshot for the new theme please? (In a followup issue is fine - just noting it somewhere).

            Show
            damyon Damyon Wiese added a comment - I tested 2 and it works as expected - so good job. Also - 4. Can we have an updated screenshot for the new theme please? (In a followup issue is fine - just noting it somewhere).
            Hide
            damyon Damyon Wiese added a comment -

            Final point: I have checked the follow-up issues, and I can see how you have used this dynamic less compilation there to e.g. import .well for the #regionmain and apply the colours - but - could this be done without dynamic less compilation by including markers in the less and doing simple replacements in the post-processing as David describes above. What is the real benefit of dynamic less here?

            Show
            damyon Damyon Wiese added a comment - Final point: I have checked the follow-up issues, and I can see how you have used this dynamic less compilation there to e.g. import .well for the #regionmain and apply the colours - but - could this be done without dynamic less compilation by including markers in the less and doing simple replacements in the post-processing as David describes above. What is the real benefit of dynamic less here?
            Hide
            damyon Damyon Wiese added a comment -

            Aha - you were right - (real life discussion) - I missed that the css_send_cached_css never returns (dies) so the lock won't be released twice.

            Show
            damyon Damyon Wiese added a comment - Aha - you were right - (real life discussion) - I missed that the css_send_cached_css never returns (dies) so the lock won't be released twice.
            Hide
            damyon Damyon Wiese added a comment -

            Also +1 for the name - I like theme_more

            Show
            damyon Damyon Wiese added a comment - Also +1 for the name - I like theme_more
            Hide
            damyon Damyon Wiese added a comment -

            Also - noting from real live chat - the reason that we have to use a less compiler here, is because if we use tokens for the colours, the bootstrap code that is using functions on the variables will explode. e.g. lighten('[[mycolorvariable]]') will not work.

            So - I think there are no more changes required here - I'll push this for integration.

            Show
            damyon Damyon Wiese added a comment - Also - noting from real live chat - the reason that we have to use a less compiler here, is because if we use tokens for the colours, the bootstrap code that is using functions on the variables will explode. e.g. lighten('[ [mycolorvariable] ]') will not work. So - I think there are no more changes required here - I'll push this for integration.
            Hide
            fred Frédéric Massart added a comment -

            Thanks Damyon,

            again, as we discussed IRL, the lessfile is NOT inherited from the parent. If a theme wants to extend another one using LESS, they have to create their own file and import the .less file of the direct parent. I enforced that because the only point of compiling LESS is to inject variables, but the variables would be set in the settings of the parent theme (in the UI), which is not helpful for your own theme (even though the callbacks are called). And if you're not willing to use any settings, then you should really extend bootstrapbase.

            I hope I made sense.

            Show
            fred Frédéric Massart added a comment - Thanks Damyon, again, as we discussed IRL, the lessfile is NOT inherited from the parent. If a theme wants to extend another one using LESS, they have to create their own file and import the .less file of the direct parent. I enforced that because the only point of compiling LESS is to inject variables, but the variables would be set in the settings of the parent theme (in the UI), which is not helpful for your own theme (even though the callbacks are called). And if you're not willing to use any settings, then you should really extend bootstrapbase . I hope I made sense.
            Hide
            dougiamas Martin Dougiamas added a comment -

            OK, so just documenting overall that we are doing this in Moodle 2.7:

            a) Leaving Clean as it is, and making it the new default. MDL-42964 MDL-41459
            b) Making a new theme called "More" with all the easy customisation stuff, including built-in LESS. MDL-43786 MDL-44357
            c) Deprecate many of the old "base-based" themes currently only there to make up for previous lack of b). This means removing them from 2.7 and moving them into moodle.org/plugins for those who still want them. MDL-43784

            Show
            dougiamas Martin Dougiamas added a comment - OK, so just documenting overall that we are doing this in Moodle 2.7: a) Leaving Clean as it is, and making it the new default. MDL-42964 MDL-41459 b) Making a new theme called "More" with all the easy customisation stuff, including built-in LESS. MDL-43786 MDL-44357 c) Deprecate many of the old "base-based" themes currently only there to make up for previous lack of b). This means removing them from 2.7 and moving them into moodle.org/plugins for those who still want them. MDL-43784
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Alrighty - so first thoughts upon looking this branch turned up the following:

            • I see that Less.php 1.7.0.1 was released 7 days ago, should be starting off with a 1.7 branch rather than a 1.6 branch?
            • lib/lessc.php ln 65 [core_lessc::import_file] it doesn't appear this will work with themes located in a custom directory as specified by $CFG->themedir (see config-dist)
            • lib/outputlib.php ln 369 [theme_config] As mentioned in my first review "All theme config properties before your change are public, the properties you've introduced are protected. I'm much rather see them being made public to achieve consistency." I'd still like to see this done.

            Fred, ping me when you're online.

            Show
            samhemelryk Sam Hemelryk added a comment - Alrighty - so first thoughts upon looking this branch turned up the following: I see that Less.php 1.7.0.1 was released 7 days ago, should be starting off with a 1.7 branch rather than a 1.6 branch? lib/lessc.php ln 65 [core_lessc::import_file] it doesn't appear this will work with themes located in a custom directory as specified by $CFG->themedir (see config-dist) lib/outputlib.php ln 369 [theme_config] As mentioned in my first review "All theme config properties before your change are public, the properties you've introduced are protected. I'm much rather see them being made public to achieve consistency." I'd still like to see this done. Fred, ping me when you're online.
            Hide
            fred Frédéric Massart added a comment -

            Hi Sam,

            • I have updated less.php to the latest version, thanks for noticing!
            • You are right, this would not work, but it was not intended to. This method is actually not used any more, but I thought it could be useful in the future. I have removed it.
            • Sorry for forgetting about that, this is done.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Sam, I have updated less.php to the latest version, thanks for noticing! You are right, this would not work, but it was not intended to. This method is actually not used any more, but I thought it could be useful in the future. I have removed it. Sorry for forgetting about that, this is done. Cheers, Fred
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Fred - this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Fred - this has been integrated now.
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            I was able to see the changes in the css on the site. However, I did notice the pages loaded extremely slow with more theme compared to clean, specially in IE browsers. Fred indicated that , this is a know fact.

            Tested on ie10, ie9 and chrome.

            Based on that am passing this issue.

            Cheers.

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited I was able to see the changes in the css on the site. However, I did notice the pages loaded extremely slow with more theme compared to clean, specially in IE browsers. Fred indicated that , this is a know fact. Tested on ie10, ie9 and chrome. Based on that am passing this issue. Cheers.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Clothes and manners do
            not make the man; but,
            when he is made, they
            greatly improve his appearance.

            ---- Henry Ward Beecher

            What a week, your changes are now part of Moodle, well done!

            Closing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Clothes and manners do not make the man; but, when he is made, they greatly improve his appearance. ---- Henry Ward Beecher What a week, your changes are now part of Moodle, well done! Closing, thanks!
            Show
            fred Frédéric Massart added a comment - Removing the dev_docs_required as I've added some documentation at: http://docs.moodle.org/dev/Themes_overview#Compiling_LESS_on_the_fly and http://docs.moodle.org/dev/Themes_overview#Theme_options_as_of_8th_April_2014

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/May/14

                  Agile