Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Themes
    • Labels:
      None
    • Rank:
      49896

      Description

      This is a split from MDL-39276

      Could we also get this as an opportunity to "normalize" the way we are going to use .less and generated .css files?

      In order to be able to "compute" (aka CI servers) all the less compilations to perform we would need something like:

      1) All the main .less files (not the included ones) to stay always under theme/xxx/less
      2) Each one of those main files will lead to, always, a .css file under theme/xxx/styles with the same name than the original (aka, moodle.less => moodle.css, editor.less => editor.css...).

      Please consider it, else it's not easily guessable (right now we are generating a "generated.css" file instead of moodle.css).

      Ciao

        Issue Links

          Activity

          Hide
          David Scotson added a comment -

          Seems like a sensible change. It mostly impacts on documentation, so probably good to make it at the same time as renaming the themes.

          (I don't think there's any LESS compiling happening on CI servers at the moment though, the output CSS is checked into git.)

          Show
          David Scotson added a comment - Seems like a sensible change. It mostly impacts on documentation, so probably good to make it at the same time as renaming the themes. (I don't think there's any LESS compiling happening on CI servers at the moment though, the output CSS is checked into git.)
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          David, the CI will have a job to recompile all the .less files and verify that the results are 100% the same than the submitted by the developers (see MDLSITE-2209). On failure, it will lead to notification to the developer and/or recompilation by integrators.

          That leads us to another "problem" that is the selection of the "definitive" tool to be used both by devs and CI servers (to be able to compare results).

          Right now it's "recess" that I've found somehow buggy (not reporting errors properly nor exit status) and unmaintained (last commit is a bunch of months ago).

          While working in the CI job above... I found the "standard" lessc one way more stable, behaving as expected (exit codes and errors), also allowing minification (own or yui-compress). Perhaps the only point not supported was the "watch" mode, not sure. Is there any reason to stay sticky to recess or could we consider moving to lessc (with a given minifier) as the "standard" .less compilation tool?

          Show
          Eloy Lafuente (stronk7) added a comment - - edited David, the CI will have a job to recompile all the .less files and verify that the results are 100% the same than the submitted by the developers (see MDLSITE-2209 ). On failure, it will lead to notification to the developer and/or recompilation by integrators. That leads us to another "problem" that is the selection of the "definitive" tool to be used both by devs and CI servers (to be able to compare results). Right now it's "recess" that I've found somehow buggy (not reporting errors properly nor exit status) and unmaintained (last commit is a bunch of months ago). While working in the CI job above... I found the "standard" lessc one way more stable, behaving as expected (exit codes and errors), also allowing minification (own or yui-compress). Perhaps the only point not supported was the "watch" mode, not sure. Is there any reason to stay sticky to recess or could we consider moving to lessc (with a given minifier) as the "standard" .less compilation tool?
          Hide
          David Scotson added a comment - - edited

          As far as I'm aware recess is dependent on lessc (or rather the npm package "less" which provides the "lessc" command line tool) to do the actual less compilation so in that sense they're using the same code for compilation.

          I think the minification options are partly 3rd party as well, with both offering YUI's CSSCompressor minifier.

          So they should be close to being interchangeable at least. Where the differences might start to creep in are recess possibly applying some of it's CSS lint changes if it thinks those will help the output gzip better, e.g. the output order of the CSS rules.

          I've not actually compared the output of the two tools (unminified or minified). What kind of differences are you seeing?

          edit: Oh, and the main reason for choosing recess over lessc was that recess is developed by the same guys that develop Bootstrap, and it's what they use and recommend. The not throwing errors properly and not providing the right exit code seem to be the same bug, and actually be a bug in LESS itself (https://github.com/cloudhead/less.js/pull/1283).

          Show
          David Scotson added a comment - - edited As far as I'm aware recess is dependent on lessc (or rather the npm package "less" which provides the "lessc" command line tool) to do the actual less compilation so in that sense they're using the same code for compilation. I think the minification options are partly 3rd party as well, with both offering YUI's CSSCompressor minifier. So they should be close to being interchangeable at least. Where the differences might start to creep in are recess possibly applying some of it's CSS lint changes if it thinks those will help the output gzip better, e.g. the output order of the CSS rules. I've not actually compared the output of the two tools (unminified or minified). What kind of differences are you seeing? edit: Oh, and the main reason for choosing recess over lessc was that recess is developed by the same guys that develop Bootstrap, and it's what they use and recommend. The not throwing errors properly and not providing the right exit code seem to be the same bug, and actually be a bug in LESS itself ( https://github.com/cloudhead/less.js/pull/1283 ).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          small minification (whitespace) diffs only, apparently. I agree compilation is the same (recess wraps lessc). I really don't know about recess "extra" features, like improving order and/or lint option.

          But there is a big difference about how recess handles errors and exit statuses (making it unsuitable for automated jobs). I think I saw an issue opened about that (with patch) since some months ago. And it never became commented/pulled upstream. (that was the main reason to consider it sort of unmaintained).

          Show
          Eloy Lafuente (stronk7) added a comment - small minification (whitespace) diffs only, apparently. I agree compilation is the same (recess wraps lessc). I really don't know about recess "extra" features, like improving order and/or lint option. But there is a big difference about how recess handles errors and exit statuses (making it unsuitable for automated jobs). I think I saw an issue opened about that (with patch) since some months ago. And it never became commented/pulled upstream. (that was the main reason to consider it sort of unmaintained).
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Clarification comments: Note that I'm not proposing to switch from "recess" to "lessc". Just putting the later on the table, to be able to compare and take (somebody with more knowledge than me) the better one.

          If finally we go to "recess", then the CI task will use it and I'll try to sort out its (exit code/error reporting) problems (surely by compiling also with lessc to look for errors properly, and later with recess to compare results). So we can accept "recess" as the "official" one. I just wanted to expose the problems I've found with it when I started (and stopped) working on MDLSITE-2209 last week).

          PS: Hehe, that issue wasn't there last week, when i looked to both recess and lessc problems.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Clarification comments: Note that I'm not proposing to switch from "recess" to "lessc". Just putting the later on the table, to be able to compare and take (somebody with more knowledge than me) the better one. If finally we go to "recess", then the CI task will use it and I'll try to sort out its (exit code/error reporting) problems (surely by compiling also with lessc to look for errors properly, and later with recess to compare results). So we can accept "recess" as the "official" one. I just wanted to expose the problems I've found with it when I started (and stopped) working on MDLSITE-2209 last week). PS: Hehe, that issue wasn't there last week, when i looked to both recess and lessc problems.
          Hide
          David Scotson added a comment -

          I diffed the outputs. As suspected, the main difference is that Recess re-orders the CSS rules into a consistent order and lessc doesn't.

          I was comparing the un-minified output so there were a couple of other whitespace differences (empty lines between rules and aligning -webkit-rules) but I assume they'll disappear after running the minifier.

          Show
          David Scotson added a comment - I diffed the outputs. As suspected, the main difference is that Recess re-orders the CSS rules into a consistent order and lessc doesn't. I was comparing the un-minified output so there were a couple of other whitespace differences (empty lines between rules and aligning -webkit-rules) but I assume they'll disappear after running the minifier.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ok David, so do you think that it's ok to continue using recess and wait for those (error reporting) issues to be fixed? Sticky to any version or not needed?

          And then, the naming schema... less/xxxx.less => styles/xxx.css. Can I assume it's going to be constantly that way? So current generated.css will disappear?

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ok David, so do you think that it's ok to continue using recess and wait for those (error reporting) issues to be fixed? Sticky to any version or not needed? And then, the naming schema... less/xxxx.less => styles/xxx.css. Can I assume it's going to be constantly that way? So current generated.css will disappear? TIA and ciao
          Hide
          Jason Fowler added a comment -

          I don't see the need for less/xxxx.less => styles/xxx.css.

          At the end of the day, designers and developers shouldn't care about what is in these css files, because it can be blown away by a recess run. The less files are where we should be looking, and they are well named and understandable.

          For performance I'd rather the one file, cached, than many files required at different times throughout Moodle. Once I have that one file, my browser never needs to request it again.

          Show
          Jason Fowler added a comment - I don't see the need for less/xxxx.less => styles/xxx.css. At the end of the day, designers and developers shouldn't care about what is in these css files, because it can be blown away by a recess run. The less files are where we should be looking, and they are well named and understandable. For performance I'd rather the one file, cached, than many files required at different times throughout Moodle. Once I have that one file, my browser never needs to request it again.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          I don't see the need for less/xxxx.less => styles/xxx.css.

          Sorry, Jason, then... what are you proposing? 100% lost here.

          .less files need to be compiled, this is exclusively about to decide some basic naming rules to let everybody (automated systems included) know.

          Currently we are doing moodle.less => generated.css that is 100% arbitrary. The proposal is, simply, to do moodle.less => moodle.css instead.

          Sorry I'm missing something...ciao

          PS: Read the description, the proposal is there, you too Mary

          MAIN FILES !!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited I don't see the need for less/xxxx.less => styles/xxx.css. Sorry, Jason, then... what are you proposing? 100% lost here. .less files need to be compiled, this is exclusively about to decide some basic naming rules to let everybody (automated systems included) know. Currently we are doing moodle.less => generated.css that is 100% arbitrary. The proposal is, simply, to do moodle.less => moodle.css instead. Sorry I'm missing something...ciao PS: Read the description, the proposal is there, you too Mary MAIN FILES !!
          Hide
          Jason Fowler added a comment -

          I thought you were saying every .less file needs a matching .css file. That is all. I don't think that is needed.

          Show
          Jason Fowler added a comment - I thought you were saying every .less file needs a matching .css file. That is all. I don't think that is needed.
          Hide
          Martin Dougiamas added a comment -

          Nope, the moodle.less main file includes all the other ones, so there would only be one in that case.

          Eloy is just talking about a policy for whatever is in the less directory to be mirrored in the styles directory (for any less-based themes making it into core). Not a big deal really.

          Show
          Martin Dougiamas added a comment - Nope, the moodle.less main file includes all the other ones, so there would only be one in that case. Eloy is just talking about a policy for whatever is in the less directory to be mirrored in the styles directory (for any less-based themes making it into core). Not a big deal really.
          Hide
          Mary Evans added a comment -

          I agree with Eloy, as this makes a lot of sense. for example: bootstrap/less/moodle/form.less => bootstrap/style/form.css (correct Eloy?)

          Show
          Mary Evans added a comment - I agree with Eloy, as this makes a lot of sense. for example: bootstrap/less/moodle/form.less => bootstrap/style/form.css (correct Eloy?)
          Hide
          David Scotson added a comment - - edited

          Mary Evans Eloy's only talking about the two "main" less files ("moodle.less" and "editor.less") which correspond to one output CSS file each, currently generated.css and editor.css. Changing the former to moodle.css is a small simplification that means a script compiling less can know what the output should be without having to be told. And if the only files in that directory are these main files, then it doesn't even need to know the titles of the input files either, it can just grab all the files in that directory.

          Your suggestion has some merit though, I have thought that about slicing the output file back into component CSS files might be an interesting idea for integrating with Moodle's current theme system. I started putting the the title of the origin file in a comment on the first line so that a script could split them easily. But if people want to look into that then it would be a different bug from this one.

          Show
          David Scotson added a comment - - edited Mary Evans Eloy's only talking about the two "main" less files ("moodle.less" and "editor.less") which correspond to one output CSS file each, currently generated.css and editor.css. Changing the former to moodle.css is a small simplification that means a script compiling less can know what the output should be without having to be told. And if the only files in that directory are these main files, then it doesn't even need to know the titles of the input files either, it can just grab all the files in that directory. Your suggestion has some merit though, I have thought that about slicing the output file back into component CSS files might be an interesting idea for integrating with Moodle's current theme system. I started putting the the title of the origin file in a comment on the first line so that a script could split them easily. But if people want to look into that then it would be a different bug from this one.
          Hide
          Jason Fowler added a comment -

          See, I thought the same as Mary, due to Eloy using "less/xxxx.less => styles/xxx.css" to describe the process.

          Show
          Jason Fowler added a comment - See, I thought the same as Mary, due to Eloy using "less/xxxx.less => styles/xxx.css" to describe the process.
          Hide
          Jason Fowler added a comment -

          The problem with doing that, is there is less/bootstrap/forms.less and less/moodle/forms.less in that folder ... that will produce a lot more files than we need. One is enough, and it can be cached easily.

          Designers shouldn't be modifying the CSS files in a LESS based theme, so we don't need to separate them out.

          Show
          Jason Fowler added a comment - The problem with doing that, is there is less/bootstrap/forms.less and less/moodle/forms.less in that folder ... that will produce a lot more files than we need. One is enough, and it can be cached easily. Designers shouldn't be modifying the CSS files in a LESS based theme, so we don't need to separate them out.
          Hide
          David Scotson added a comment -

          Yeah, Eloy was being very precise in specifying only the files in that folder and not sub-folders, but I read it the same way as Jason and Mary the first time, I think possibly I only got it because it was me that set up that folder structure in the first place.

          For the discussion of splitting the CSS files (which isn't a 2.5 thing but interesting to talk about) it shouldn't really matter how many files you end up with. I think there's like 60 or so when you're in theme designer mode at the moment, and they all get compiled down to 1 (or 3 for IE) in production. So a few extra CSS files wouldn't hurt. The benefits would be that child themes that implement tabs differently could exclude the tabs CSS from the parent theme rather than just override it, which leaves the existing CSS in place so it still gets downloaded, but I don't know how much benefit from that modularity we'd get at the moment.

          Show
          David Scotson added a comment - Yeah, Eloy was being very precise in specifying only the files in that folder and not sub-folders, but I read it the same way as Jason and Mary the first time, I think possibly I only got it because it was me that set up that folder structure in the first place. For the discussion of splitting the CSS files (which isn't a 2.5 thing but interesting to talk about) it shouldn't really matter how many files you end up with. I think there's like 60 or so when you're in theme designer mode at the moment, and they all get compiled down to 1 (or 3 for IE) in production. So a few extra CSS files wouldn't hurt. The benefits would be that child themes that implement tabs differently could exclude the tabs CSS from the parent theme rather than just override it, which leaves the existing CSS in place so it still gets downloaded, but I don't know how much benefit from that modularity we'd get at the moment.
          Hide
          Damyon Wiese added a comment -

          It seems we agree with Eloys suggestion to rename generated.css to moodle.css (and generating separate css from all the files in less/moodle is another issue).

          Trying to clear blockers from renaming bootstrap to bootstrapbase so - here is a patch for this issue.

          Show
          Damyon Wiese added a comment - It seems we agree with Eloys suggestion to rename generated.css to moodle.css (and generating separate css from all the files in less/moodle is another issue). Trying to clear blockers from renaming bootstrap to bootstrapbase so - here is a patch for this issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          I ended cleaning the incoming generated.css & moodle.css changes coming in your patch and added extra commit to regenerate it manually (it was conflicting in a really strange way because of changes an moves together).

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! I ended cleaning the incoming generated.css & moodle.css changes coming in your patch and added extra commit to regenerate it manually (it was conflicting in a really strange way because of changes an moves together).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing, clean looks correct after the move, so surely everything is being picked of from new location and generation. Yay!

          Show
          Eloy Lafuente (stronk7) added a comment - Passing, clean looks correct after the move, so surely everything is being picked of from new location and generation. Yay!
          Hide
          Dan Poltawski added a comment -

          Thanks! You're changes are now spread to the world through this git and our source control repositories.

          No time to rest though, we've got days to make 2.5 the best yet!

          ciao

          Show
          Dan Poltawski added a comment - Thanks! You're changes are now spread to the world through this git and our source control repositories. No time to rest though, we've got days to make 2.5 the best yet! ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: