Skip to main content ↑ ↓
Topic: Proposed Fix For Variants CSS issues (Read 11432 times) previous topic - next topic
0 Members and 1 Guest are viewing this topic.

Re: Proposed Fix For Variants CSS issues

Reply #15 –

BTW, this is overly simplistic:

Quote from: emanuele – The current structure is:
"general" (current index.css) or non-variant gives the structure,
 variant gives the colors.
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.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Proposed Fix For Variants CSS issues

Reply #16 –

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.

Re: Proposed Fix For Variants CSS issues

Reply #17 –

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).
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Proposed Fix For Variants CSS issues

Reply #18 –

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:
Code: [Select]
loadCSSFile('index');
should load that order:
Code: [Select]
/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).
Bugs creator.
Features destroyer.
Template killer.

Re: Proposed Fix For Variants CSS issues

Reply #19 –

Quote from: emanuele – TBH, at that point I'd see better something that loads all the files that finds starting from default back to the variant...
So:
Code: [Select]
loadCSSFile('index');
should load that order:
Code: [Select]
/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.

Re: Proposed Fix For Variants CSS issues

Reply #20 –

Quote from: emanuele – 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. ;)
I'm only changing the definition of fallback to have one intermediate level.

Here's my suggested pseudocode.
Code: [Select]
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.

Re: Proposed Fix For Variants CSS issues

Reply #21 –

Quote from: scripple – And you're loading js that way too.  js doesn't undo itself like that.
Let's not worry about it for the moment.
If done "properly"[1] the two will behave differently just by setting a flag.

Quote from: scripple – 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.
When I see code, I read that because it's usually (to me) easier to understand than English... lol Sorry... :-[

Quote from: scripple – I'm not trying to radically alter your variants just make them more versatile.
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"[2] 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:
Code: [Select]
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:
Code: [Select]
/my_theme/index.css
/my_theme/_variant/index.css
or
Code: [Select]
/my_theme/index.css
or
Code: [Select]
/my_theme/_variant/index.css
if the file exists in the variant and/or in the base, otherwise:
Code: [Select]
/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.
1 Then probably I'm not the right person to code it. LOL↵
2 The definition of simple is subject to interpretation.↵
Bugs creator.
Features destroyer.
Template killer.

Re: Proposed Fix For Variants CSS issues

Reply #22 –

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.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Proposed Fix For Variants CSS issues

Reply #23 –

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.

Re: Proposed Fix For Variants CSS issues

Reply #24 –

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.

QuoteI recall seeing color and structure in both the main and the variant.
That's a cock-up. and should be fixed before Final.

QuoteSo 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.)
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.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Proposed Fix For Variants CSS issues

Reply #25 –

Quote from: Antechinus – 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.

Re: Proposed Fix For Variants CSS issues

Reply #26 –

How about this.  It should keep people happy.  No special cases but the separation still exists. 

Pseudo-code:
Code: [Select]
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.
Last Edit: April 09, 2014, 11:33:07 pm by scripple

Re: Proposed Fix For Variants CSS issues

Reply #27 –

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.

Re: Proposed Fix For Variants CSS issues

Reply #28 –

Okay, I reverted my commit[1] 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:
Code: (sidebar.css (base)) [Select]
.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:
Code: (sidebar.css (dark)) [Select]
.sidebar {
    width: 12em;
    margins: 1em;
    background-color: #000;
}
Basically a copy of the entire file, because the only css loaded will be:
Code: [Select]
/my_theme/_dark/sidebar.css

With my approach:
Code: (sidebar.css (dark)) [Select]
.sidebar {
    background-color: #000;
}
just the color, because the stack of files loaded will be:
Code: [Select]
/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. ;)
1 Commit archived here for reference, but I suppose it can go.↵
Bugs creator.
Features destroyer.
Template killer.

Re: Proposed Fix For Variants CSS issues

Reply #29 –

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.