So I've been campaigning for fixes to the variant system not loading the css files for the editor. I finally decided to do something about it. I've rewritten loadCSSFile with the following changes.
New input parameter 'variant'. If set to true (the default) and $context['theme_variant'] is set loadCSSFile attempts to load the css file from the variant directory. If the file does not exist there it falls back to base directory of the default theme. (If you're up for this patch this can be fixed to fall back to the base of the current theme and then the base of the default theme.)
Otherwise the behavior is what it already was supposed to be.
However for this to work well I suggest we drop the _variant name from css files in the variant theme directory. It's redundant and requires a bunch of unnecessary name mangling. The different directory is sufficient to make it unique.
As it turns out index.css is loaded before $context['theme_variant'] is set so it automatically loads from the top level directory with no other changes. But to make it less fragile the variant input parameter can be set to false for loading index.css and anything else that you want to always load a base copy of. And to not have the base files loaded twice for the things you intend to have live beside the base set fallback = false.
So _light/index_light.css would become just _light/index.css and the index css calls would be
loadCSSFile('index.css', variant = false) <= load the main index.css
loadCSSFile('index.css', fallback = false) <= Only call this if there are variants. Will load a _light/index.css for example but not reload the main index.css if it doesn' t exist.
This would also let the hack for loading variant files for admin. If a variant is set it would automatically load the one from the variant otherwise it would fall back to the default. So people who don't want to theme the admin don't have to have an admin file in their variant directories anymore.
This also lets any css file (other than the sceditor iframe which has already been handled separately) be overwritten by a variant while falling back to the base if present. (Currently only for the default as there needs to be an outer fallback for non-default themes. I can write that if you're willing to go forward with this.) (And yes, I updated the cache hash so multiple variants can be in use at once.)
And as an aside, while doing this I found a few bugs in loadCSSFile. The check for fallback = false doesn't actually work if you set fallback=false. And if you pass in multiple file names to loadCSSFile in one call then the first one that triggers fallback will automatically cause all following files to be loaded from the default theme whether they exist or not.
So what do you think? Is this change acceptable?
Here's the modified function.
/**
* Add a CSS file for output later
*
* @param mixed $filenames string or array of filenames to work on
* @param mixed[] $params = array()
* Keys are the following:
* - ['local'] (true/false): define if the file is local
* - ['fallback'] (true/false): if false will attempt to load the file
* from the default theme if not found in the current theme
* - ['variant'] (true/false): if true will attempt to load the file
* from the theme variant falling back to the default theme if not available
* - ['stale'] (true/false/string): if true or null, use cache stale,
* false do not, or used a supplied string
* @param string $id optional id to use in html id=""
*/
function loadCSSFile($filenames, $params = array(), $id = '')
{
global $settings, $context, $db_show_debug;
if (empty($filenames))
return;
if (!is_array($filenames))
$filenames = array($filenames);
// Static values for all these settings
$params['stale'] = (!isset($params['stale']) || $params['stale'] === true) ? CACHE_STALE : (is_string($params['stale']) ? ($params['stale'] = $params['stale'][0] === '?' ? $params['stale'] : '?' . $params['stale']) : '');
$params['fallback'] = (isset($params['fallback']) && ($params['fallback'] === false)) ? false : true;
if ((isset($params['variant']) && ($params['variant'] === false)) || empty($context['theme_variant']))
{
$params['variant'] = false;
$params['cache_dir'] = $settings['theme_dir'];
$params['dir'] = $settings['theme_dir'] . '/css/';
$params['url'] = $settings['theme_url'] . '/css/';
}
else
{
$params['variant'] = true;
$params['cache_dir'] = $settings['theme_dir'] . $context['theme_variant'];
$params['dir'] = $settings['theme_dir'] . '/css/' . $context['theme_variant'] . '/';
$params['url'] = $settings['theme_url'] . '/css/' . $context['theme_variant_url'];
}
// Whoa ... we've done this before yes?
$cache_name = 'load_css_' . md5($params['cache_dir'] . implode('_', $filenames));
if (($temp = cache_get_data($cache_name, 600)) !== null)
{
if (empty($context['css_files']))
$context['css_files'] = array();
$context['css_files'] += $temp;
}
else
{
$this_build = array();
// All the files in this group use the parameters as defined above
foreach ($filenames as $filename)
{
// Account for shorthand like admin.css?xyz11 filenames
$has_cache_staler = strpos($filename, '.css?');
$params['basename'] = $has_cache_staler ? substr($filename, 0, $has_cache_staler + 4) : $filename;
$this_id = empty($id) ? strtr(basename($filename), '?', '_') : $id;
// Is this a local file?
if (substr($filename, 0, 4) !== 'http' || !empty($params['local']))
{
$params['local'] = true;
// Fallback if needed?
if ($params['fallback'] && ($params['variant'] || ($settings['theme_dir'] !== $settings['default_theme_dir'])) && !file_exists($params['dir'] . $filename))
{
// Fallback if we are not already in the default theme
if (file_exists($settings['default_theme_dir'] . '/css/' . $filename))
{
$filename = $settings['default_theme_url'] . '/css/' . $filename . ($has_cache_staler ? '' : $params['stale']);
}
else
$filename = false;
}
else
$filename = $params['url'] . $filename . ($has_cache_staler ? '' : $params['stale']);
}
// Add it to the array for use in the template
if (!empty($filename))
$this_build[$this_id] = $context['css_files'][$this_id] = array('filename' => $filename, 'options' => $params);
if ($db_show_debug === true)
$context['debug']['sheets'][] = $params['basename'] . (!empty($params['url']) ? '(' . basename($params['url']) . ')' : '');
// Save this build
cache_put_data($cache_name, $this_build, 600);
}
}
}
And yes this would require a few changes elsewhere in Load.php to drop the _variant part of the file names.
This is sounding good. Will have to see what the other deranged wombats think.
And amusingly enough since the variants and name mangling are currently added outside of calls to loadCSSFile this actually works fine with the current structure. I'm still for simplifying it though and dropping all the manipulation outside of loadCSSFile.
I didn't read that one until now several hours ago because I thought it was one I already read and was keeping unread to remember something... lol
Anyway, I posted this issue before reading this topic:
https://github.com/elkarte/Elkarte/issues/1525
that is somehow related to your second post.
I'm looking at that part of the code now, I agree on moving variants to loadCSSFile, though there is a problem (as usual): loadCSSFile may be called before the variants are set (and I'm not only talking about the "core", but addons too), so we'd have to separate the "adding" from the "processing" (intended as the code that checks if the file exists or not), moving the processing much further, ideally just after the point I moved the loading of custom.css.
I already found another couple of bugs in the two functions loadCSSFile and loadJavascriptFile, and in the combine class and the templating, I fixed them (see https://github.com/emanuele45/Dialogo/compare/master...fivefixes ), but add some more code now sounds somehow risky.
Additionally, I'm a bit concerned by the fallback ability, that one has a meaning only for non-variant files, because I feel it's unlikely two themes will have compatible variants (for example even only in respect to the name, let alone the css) and I'm not sure how it works in real-life cases.
I'd say this is food for 1.1, and much needed. ;D
Another thing I would consider is to simplify the stuff: for example "force" themes to have at least one variant, it's just a matter of keep some files into a specific directory.
Dunno, looks like there are tons of alternatives, even too many... :-\
Actually... you are right scripple: it may not be as tricky as I thought.
Changing it to fallback only from variants to the current theme base would be trivial. That's really what it's doing now all that would be needed is drop default_ prefixes from the fallback and replace them with the current stuff.
The calls before the variant is established I thought was a feature. :)) But as long as user_id is set before the first call it wouldn't be too hard to make it lookup the desired variant itself if the variant flag is set. And the variant flag could still be set false to force loading of theme base files like index.css.
In the existing system any calls made before variant is set right now obviously get the base theme files. They still do with this in place. Actually I saw index.css and fontawesome.css get loaded before the variant was set every time. It works w/o issue happily finding the base version.
Really this isn't that big of a change. I've been running it on my test forum for a couple days now and the existing hardcodes all pass though the same way they would before. I'd have to look at your minimizer to see if it needs a little tweaking or not. And if you're married to the vairant/file_variant naming it's not really hard to fix that here if we assume css files will come in without the variant already appended. (And that assumption means that that existing calls to variant files will still work.) I still think the variant/file_variant is redundant. Like it came about before it was decided to use directories and everything lived in one directory.
I don't find this very frightening myself. loadCSSFile already needs tweaking (won't have time to look at your changes this morning) and certainly outside the minimizer this is easy to test. (Haven't looked at that yet either.) print statements from loadcss actually make it pretty easy to see what's being requested and what's being loaded.
Really this is kind of doing the hardcoded tweaks like the ones for admin non-wiz sceditor without requiring hard coded external tweaks. There's basically no difference between you setting the directory before you call loadcss or having loadcss do it for you in terms of results. And in case there was some edge case where you didn't want the variant added by default that's why I gave it a flag to force disable that feature on a call.
I committed this:
https://github.com/emanuele45/Dialogo/commit/26f9192d6547d253156fa230d03ffcd652152475
(with your name because the code changes are a variation of yours, if you don't want just let me know ;)).
I think in this function there is still a bug somewhere (not exactly sure where and if it actually is a but, just a feeling), .
If left this way yes please take my name off the submit.
I think this is the wrong behavior regarding variants and fallback. You've made it default to not searching variants and to falling back to the default theme. It should default to searching variants and fall back to the base of the current theme. The point is to make variants basically be able to overwrite any css by default (anywhere loadcss or a template style sheet is given) but not have to override anything the variant doesn't need to. By making the variant call explicit and not letting it fall back to the base theme it defeats the purpose. Now again every variant search will have to be explicitly coded in all of Elk and all extensions for variants to be able to change things. And if someone makes a new theme with variants they'll be required to have variant files for everything that makes a variant enabled call because otherwise it will not fallback and the load will fail since you disable fallback if a variant search is made.
To me a variant falling back to base is much more important and natural than a completely separate theme falling back to default theme. But I guess if you're going to fall back from a theme to the default then there really should be two levels of fallback. Variant => Base => Default Base.
If a whole theme can fall back on the default certainly the themes minor variants should be able to as well. Please, let's do two levels of fallback or drop the fallback to the default theme. But we have to bring the variant fallback by default back as that's the whole point of this change. So atwho, or sceditor, or admin, or anything anyone adds can all be themed in a variant if appropriate or ignored if not and safely fall back to the base of that theme.
And a minor bug. When you construct the final filename when not falling back you mix url and dir settings.
$filename = $settings['theme_url'] . '/' . $params['subdir'] . $variant_dir . '/' . $filename . $cache_staler;
I think that should be variant_url and a leading slash not trailing since the variant urls get the trailing slash already.
I agree with Scripple here about the levels of fallback.
Thinking a bit more, since I know emanuele is a little worried about falling back from another theme's variant all the way to the base of the default theme let me try to say simply why there's no reason to worry. The only reason it would fall back that far is the person making the theme didn't change things enough in either their theme base directory or their variant to create the file. So it's obviously not a problem for them that it falls back. If the theme or variant was somehow so different that they had to change whatever is missing, well then it wouldn't be missing would it? And the fallback would never happen.
Really unless there's a bug in the final version this won't break things. If the variant doesn't have the file it will load the same one it's been loading all along. If there is a bug in the final version, it will show up really fast as suddenly several js or css files don't get loaded and everything is broken.
And you'll be able to rollback some of the "check for variant and load it if it exists stuff" you just put in. Everything can be handled by a default call to loadCSSFile or loadJavascript. The only special one is the iframe styling on the sceditor. And I'd say to be sure the default theme top level index.css gets loaded once and only once no matter any future code shuffling that the calls to loadCSSFile for the base and variant index.css files should set variant=false on the base call and fallback=false on the variant call. Everything else should let variants do their thing. And once you're happy it hasn't caused the end of Elk we can rename the files in the variants dropping the _variant part of the file name and drop all the special check and load variant items.
And once this is place even mods can be styled by variants without work by the mod author. That way when Ant finally gets so frustrated with the word filter and writes the badwords mod that injects random bad words in everyone's profiles and signatures I will be able to create a file in my dark variant's css directory called badwords.css and @include his original but fix the coloring to work on the dark background. And I don't have to go whine to him about making a dark theme variant of his mod.
I can just post naughty images of words. :D
At the moment I can't read and understand everything so forgive me if I write something that has already been discussed.
The code I committed doesn't really do any decision, it follows the instructions.
Fallback was always meant to be fallback from a theme to default. If the idea is to change that meaning, then I missed it.
I'm starting from the idea (maybe wrong) that each call to loadCSSFile loads one file. In the sense that loadCSSFile('index.css') loads 1 css and not the variant css, and then the general css, I'm not sure if this is a common ground or you have in mind something different. :)
That said, it's not that I'm worried about falling back from a variant to the "general" and then the default, is that I see no meaning in doing so, at least the way the theme is currently structured.
The current structure is:
"general" (current index.css) or non-variant gives the structure,
variant gives the colors.
With that subdivision in mind, if we fallback from variant to "general" we get something completely different than what we are aiming at.
What's the point of loading something that gives structure when we want colors?
Then fallback from variant to default. Following your logic the natural flow of fallbacks should be my-theme-variant => my-theme-non-variant => default-theme-variant => default-theme-non-variant.
Fine: you theme has variant: my_nice_holidays.
How can I fallback from that to dark? Or maybe it was in your variant you wanted to fallback to besocial?
It's not that I'm worried about falling back to a "default" variant, is that it's technically impossible because unless you in the theme declare a "dependency" Elk cannot know where you want to fallback. Of course you can say: "just fallback to the same name", fine, what if tomorrow I want to have two black variants in my themes? black and black2? balck2 will never be able to automatically fallback to the default variant.
P.S.
There are just questions, not attacks or a kind of defense. ;)
I'm not saying anything about going back to default theme variants for exactly the reason you stated. Not likely to have the name. And in fact I'm fine with falling back no farther than the base directory of the active theme. It's actually what we discussed a few posts up before your version.
Two big changes you made I disagree with.
1) You made the default setting, if variant isn't set, to be variant=false. It should be the other way. Default to variant=true unless the user specifically sets it false.
2) You did this: if (variant == true) fallback = false. Variant should have no effect on fallback. Fallback should default to true like it always has and be independent of variant.
Fallback should happen from variant to the base of the current theme. For example besocial and light both have happily shared many js and css files. No reason to prevent that. (And since this now handles css and js it's very important to fallback to the theme base directory.)
I'm not personally worried about falling back all the way to the default theme, but you had it in there before this mod and put it in again so I assumed you want it.
So my wish:
if (empty($context['theme_variant']) || (isset($params['variant']) && ((bool) $params['variant'] === false)))
{
$variant_dir = '';
$variant_url = '';
}
else
{
$variant_dir = '/' . $context['theme_variant'];
$variant_url = $context['theme_variant_url'];
}
And don't make fallback related to variant at all
/ * REMOVE THIS
// Always ignore fallback if using a variant
if (isset($params['variant']) && ((bool) $params['variant'] === true))
$fallback = false;
*/
// Of course no fallback if it is unwanted
/* Dropped else here as the top condition is gone
elseif (isset($params['fallback']) && ((bool) $params['fallback'] === false))
*/
if (isset($params['fallback']) && ((bool) $params['fallback'] === false))
$fallback = false;
// In any other case: fallback to default theme
else
$fallback = true;
And fall back to the current theme not the default
if (file_exists($settings['theme_dir'] . '/' . $params['subdir'] . $filename))
{
$filename = $settings['theme_url'] . '/' . $params['subdir'] . $filename . $cache_staler;
$params['dir'] = $settings['theme_dir'] . '/' . $params['subdir'];
// Doesn't the URL need the subdir too????
$params['url'] = $settings['theme_url'];
}
These were hastily typed in the editor. There are likely bugs or typos but hopefully it makes it clearer what I'm aiming for.
Actually with this doing js it might still be good to fall back to the default theme base directory if the current theme base directory doesn't have the file. But no, never fallback to default them variants. Again remember this now works for javascript not just css. And a lot of variants and theme probably won't use custom javascript.
BTW, this is overly simplistic:
Variants can also be used to change structure, or at least give the appearance of changed structure by changing the presentation of the markup. An obvious current example is the fixed top menu and reshuffled board index in the besocial variant.
I would expect that in practice the variant system would be used to give additional flexibility to aspects of presentation other than just colours. In fact, in some ways I think this is a better use for it.
Well if the change to make loading variant files the default is accepted given that this now applies to loading CSS and Javascript I'd say it gives people a lot of ability to do many things in a variant, but conveniently doesn't require them to do hardly anything if they don't want to. Who knows what people will do with it but it's nice to give them that option.
Currently I see 12 css files in the default theme css directory and four in the css/_light directory. Who knows which of those 12 someone might need to tweak in a variant. That's the problem with making specific exceptions piecemeal and the reason I wrote the version I posted.
Oh I totally agree. The variant system has to be able to handle anything in the CSS anywhere or it will end up driving people nuts.
My ruminations were more to do with the (unavoidable) greater complexity of the file system in Elk compared to 1.1.x or 2.0.x, and the resulting greater complexity of getting wild with colour variations. I've been sorta thinking it would make as much sense to use the variants system to give layout variants of a theme, with different colour variants done as separate themes. It's something I'd have to play around with a bit to get a real feel for the best all-round options.
Plus, of course, there's still the option of manually coding your own superimposed options in index.template.php so you can effectively add variants to variants (sounds bonkers, but have done it before - can be handy occasionally).
I have been kicked out from the internet for a day or so... :(
At least now I understood that the meaning you give to the term "fallback" is different from the one I gave it.
I was partially misled by the original definition of "fallback" in Elk terminology and by code you posted (that doesn't actually do a full fallback, but just picks one between the base and the variant for the current theme and if that doesn't exist falls back to default base), but whatever, not important now. ;)
Okay, I think I got it now what you really want.
It sill look a bit overkill to me, but I'm not a themer, so if you feel that's the way to go, do it is just a matter of add at least another file_exists (that anyway are cached, so should not impact too much on performances).
TBH, at that point I'd see better something that loads all the files that finds starting from default back to the variant...
So:
loadCSSFile('index');should load that order:
/default/css/index.css
/my_theme/index.css
/my_theme/_variant/index.css
in that order, that looks to me more useful (should generate less duplication if you just want to tweak a variant for colors or position).
Please don't do it that way. Unsetting things you never wanted set can be a pain. And you're loading js that way too. js doesn't undo itself like that.
Let's just add the second layer of fallback. I know it wasn't in the original code I posted. It was a quick floater to see if it would fly here. I said I'd add the to base theme first step if the idea was accepted.
I'm only changing the definition of fallback to have one intermediate level.
Here's my suggested pseudocode.
if(variant_file_exists) {
load it
} else if (current_theme_base_file_exists) {
load it
} else if default_theme_base_file_exists) {
load it
}
I'm not trying to radically alter your variants just make them more versatile.
Let's not worry about it for the moment.
If done "properly" the two will behave differently just by setting a flag.
When I see code, I read that because it's usually (to me) easier to understand than English... lol Sorry... :-[
But if it is not useful it make sense to change it.
And since we are in the process to change it, I would like to avoid as many exceptions as possible (and for example index.css would be an exception).
Can I ask some more questions?
You said no to the "load everything". Fine, TBH I didn't like it either.
But of course I have another proposal. :P
A variant should usually be a "simple" variation of the base theme. Somewhere in the theme something is defined and the variant "extends" (or changes) it. Am I right up to here?
If so, let's go a bit further.
So, a variant will always need the "base" (otherwise it would be easier to just create a new theme instead of loading twice the css just to replace it on-the-fly).
What about this:
if (current_theme_base_file_exists)
{
load_it;
$loaded = true;
}
if (variant_file_exists)
{
load_it;
$loaded = true;
}
if (!$loaded)
{
load default_theme_base_file
}
The net result may be:
/my_theme/index.css
/my_theme/_variant/index.css
or
/my_theme/index.css
or
/my_theme/_variant/index.css
if the file exists in the variant and/or in the base, otherwise:
/default/css/index.css
if the file doesn't exists in the "my_theme", but exists in default theme.
I'm modelling this exactly around index.css, and how it is used because in fact is the more generic file in the lot, but should work as any other file.
The problem is your second footnote. :) Something as simple as a dark variant really requires theming the wysiwyg too, which is how we ended up in this mess.
Well, if we're discussing a major change and want index.css to not be an exception, I'd personally rather have it still go variant=>current base=>default base and not split index.css into a base part and a variant part.
I'm not sure what that split is supposed to do actually. I recall seeing color and structure in both the main and the variant. But obviously the majority of css files are happily shared between _light and _besocial since it's a 12 to 4 split between base and variant dir. So I'd rather have no file ever load more than one copy.
The css is just a part of the theme. A lot of it is also all the php templates and all of the js scripts. So having variants that have very different css is not necessarily a good reason to split it into two themes.
So I'd drop the base theme index.css and only have it exist in the variants if I was going to get rid of the exception. (Yes, you'd have to merge the base index.css into the two existing variants.) Or make the base index.css be the current base index.css and index_light.css combined (since I think _light is still the default variant) and remove index_light.css. Then people using index_light.css would just load the main index.css and people using just besocial would load just index_besocial.css (which has now been combined with the parts of index.css it didn't overwrite.)
This also has the advantage that you don't have to make a special case for javascript. (Which if you're going to do that you should just go back to two separate functions and ditch loadAsset.)
Now you have no special exceptions and variants can override any css or javascript file if they want, but don't have to override any of them. Which would mean if I was creating another light them probably all I would have to do is copy either _light/index.css or _besocial/index.css to my variant and modify those.
But if we keep the index files separate (and someone had a reason to that) I'd still prefer that index.css be the one special case and that otherwise nothing loads two files. I think the two files leads to a lot of css bloat.
The idea of the split between colour and structure was to make it easier for beginners to customise their site by using their own choices of colours, etc. It means smaller files for them to dig through, whcih they'll find less intimidating. It also means they hopefully wont be tempted to frig around with the structure until they know what they are doing.
Markup controlled by CSS can be immensely flexible, but can also be incredibly difficult to work with if you have no idea what you're doing. This was a problem with 2.0.x theming. It was just too brain-frazzling for the average beginner. Colours they can deal with. Floats and stuff is too much for them.
The other bonus with splitting colour and structure is that it makes it easier to do several minor colour variants on the one theme (like blue category bars instead of red, or whatever) which is a pretty popular way of introducing variety into a theme.
That's a cock-up. and should be fixed before Final.
That would require a major rewrite of the besocial,css because it would then have to override a lot more stuff from the light.css. IOW, it would dramatically bloat the files for any variants.
No it wouldn't have to overwrite anything, as the top level index.css would never be loaded for besocial. So it would just have to include the current info from index.css at the time of the change (and probably end up smaller in totally actually). The point was emanuele wants no special cases so I said I'd rather the two index.css files be combined rather than have EVERYTHING load files from both the variant and the base. So yes every variant's index.css would be bigger, but it was be the only index.css loaded so it's not like the total download for any user would be bigger.
If the desire is to keep index.css split into two files than I still think my original suggestion is the way to go. So index.css has to be special cased, the function I proposed was designed to support that. And that's fewer special cases than exist right now and it's a lot more flexible.
How about this. It should keep people happy. No special cases but the separation still exists.
Pseudo-code:
rename('_light/index_light.css', '_light/variant.css')
rename('_besocial/index_besocial.css','_besocial/variant.css')
loadCSSFile('index')
loadCSSFile('variant')
.
.
.
function loadCSSFile(file)
loadAssetFile('css', file)
}
function loadAssetFileFile(index, file)
{
if (file_exists($current_theme/$variant/file))
{
load($current_theme/$variant/file);
return;
}
if (file_exists($current_theme/file))
{
load($current_theme/file);
return;
}
if (file_exists($default_theme/file))
{
load($default_theme/file);
return;
}
// If we get this far the file doesn't get loaded
}
Now there are no special cases. The separation between the structure and the colors still exists. Everything can still be overridden. If a theme doesn't have a variant the call to loadCSSFile('variant') will just fail. No problem.
If someone wants to ditch the main index.css and move it into their variants, no problem. They can have index.css in their variant directory and it will load from there instead of the base. The can still have a variant file or not.
The only thing that needs to be changed in the existing files is rename all the files in the _light and _besocial directories. Drop the _XXX from them all and rename index_XXX.css to variant.css.
This also makes it less confusing for beginners in any documentation as you don't have to worry about them touching the wrong index.css file. You tell them to modify variant.css in their variant directory. And yes that is the world "variant" not the name of their variant. (Someone have a better name? appearance.css? beginners.css? idontreallycarewhatnameaslongasitisnotindex.css)
All special case loadCSSFile calls are changed to just loadCSSFile(basename). Except the special case where the index_variant was loaded is now changed to loadCSSFIle('variant').
Ok?
ETA: Actually under this scheme I don't even see a reason to keep the fallback and variant parameters around. Just always implement the two level fallback. People who don't want it to fallback can just use different names. Certainly the variant parameter can go in this scheme.
Quick question. If our licenses were compatible in the near future... Would you look into reusing the Wedge skin system? That would solve all of your problems... Smf's Css variants suck. Nuff said.
Okay, I reverted my commit and I'll PR the stuff because anyway is needed to fix bugs.
I'm now quite confused as to what people expect the variants to be, I see ideas floating around, and on something as important as theming I don't want to push a decision and revert it tomorrow (i.e. use something on 1.0 and something completely different on 1.1). So, while working on 1.1 there will be plenty of time to define exactly what's the best way for variants to behave.
I think
@TE has some ideas too and a direction in mind. Now he is a bit busy, but when he will have more time we can hear from him his ideas.
What I see up to now is:
current approach, reduce css duplication, requires some more css in variants if the goal is a bit wider than "allowed" (see besocial),
proposed changes 1 (fallbacks), would require more css duplication (in fact each variant would be a different theme, or at least would require more css),
proposed changes 2 (partial fallback <= the last I proposed) would mostly mimic the current behaviour of index.css (loading any variant first and then the base),
proposed changes 3 (use different name for variants) would require to explicitly load variants "required" (index_variant (now renamed to variant.css), custom_variant.css, rtl_variant.css), or alternatively move the entire css file to the variant and do the changes (so everything would be overridden and require some more css duplication).
The main problem I see in this approach (but I may be badly wrong, as usual I'm not a theme guy), is that
if the variant is just a simple variation of the "base" theme, if I load
only the variant file (speaking for example about a mod), I will have to copy all the existing css, and if I want to do a small change to an element in both variants I'm for sure forced to do it in both the files, while with the alternative approach I proposed, it may be enough to add the change to the "base" css.
For example, we are talking now about a mod that adds a css file named "sidebar.css" that contains:
.sidebar {
width: 12em;
margins: 1em;
background-color: #FFF;
}With your approach, if I have a light and a dark variant I can let this be for the light variant, in the dark I'll need:
.sidebar {
width: 12em;
margins: 1em;
background-color: #000;
}Basically a copy of the entire file, because the only css loaded will be:
/my_theme/_dark/sidebar.css
With my approach:
.sidebar {
background-color: #000;
}just the color, because the stack of files loaded will be:
/my_theme/sidebar.css
/my_theme/_dark/sidebar.css
Now, if you want to change the width of the side bar, with your approach you have to do it in two places: the base sidebar.css and the dark variant sidebar.css.
With mine: you just change the base.
Of course this is just an ideal example.
I'm not sure if something like that is meaningful.
I'm not sure if, from a themer point of view is better to change it twice or once. That's why I'm pushing my view. If you are sure that people creating themes are happier with repeating changes in multiple variants, then fine, then I'd go as far as to say: drop the variants and remove the requirement of index.template.php for new themes and create a new theme each time you want to change color. It would be much easier I think: one css, done.
@Nao to be honest I don't know. I don' know enough the system to judge (yes, I read about it now and then, but reading about it is different from actually using it and understand it), what I can say is that personally I'm not a fan of xml (I seem to remember there is an xml skeleton somewhere), but that's just me. ;)
Ok, I don't like the two layer approach but it's trivially easy to do in my last proposal if that's what the variant writer wants.
For your sidebar example if I create my own sidebar.css file which I'm going to have to write code to load somewhere (in a hook probably) and I want to have a base/variant split all I have to do is loadCSSFile(array('sidebar','sidebar_variant') and suddenly I've got the split approach and don't have to worry about keeping files in sync. But if I don't want the split behavior it's not forced on me.
If I want to create a split style override of an existing css file in the base theme you can do that with a hook too. The pre_css_output hook puts those files after all the others so I can use that hook to do, using sceditor as an example, loadCSSFile('sceditor_variant') in the hook. Then you get the split style. And if you only have one variant (say a dark one) that needs to override sceditor you only need to have the file exist in that one directory.
So with my latest approach you can easily go either way and it's minimal changes to your existing code and it eliminates all the special cases (other than the sceditor wiz) that currently exist.
Let's just go with this.
It's more in keeping with the way you're doing it now. And looking at a few of the css files you're right the split is a better way to go. And if a particular file is just too annoying to do the split approach the non-split effect is trivial to achieve by just deleting the base copy. Easier than hooks and such.
So yes, this is definitely the easier and more flexible approach. I vote we go with it.
IIRC the existing system (meaning the one that existed a couple of weeks ago) was generally fine, with the exception that the WYSIWYG couldn't be themed to take account of dark variants. I wasn't aware there were any other problems that required fixing.
That code above looks right to me though. It's the way themes have always worked (or should have if the variant system wasn't incompletely implemented in 2.0.x) so will be familiar to people, and it covers all bases.
So there's still no way to style the non-wiz editor in the current master. Nothing in this thread is implemented and the other fix to load a variant file for jquery.sceditor.css isn't there either.
ETA: Nevermind. Just stupidity on my part.
Kindly bumping my question ;)
I have not looked at the skin system you put in place either :( to know how it works or how it could be used in ElkArte. I'm so far behind on so many things !
I think it would be very nice if there was some synergy between the two forks, allowing themers / addon writers to be able to create a theme that with little changes could work in two places, same for addons. Realistically I don't know how that can happen (even a basic common hook naming scheme seems difficult) but I know that no matter how good a job we all do, users will want to customize the final products. Its that body of customizations that really help the usage / acceptance level of software as well.
I'd be willing to allow ElkArte to reuse both Wedge-only features that are skins and plugins. I'm not willing to do the work for you guys though... So it might not be worth it for you.
I couldn't care less about SMF but Elk and Wedge being developed apart from each other is quite a bummer. They both are priceless in their own right. (especially W***e but let's not get into it :D)