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...)
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.
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...