Financial-Times / o-grid

Responsive grid system
http://registry.origami.ft.com/components/o-grid
94 stars 17 forks source link

Provide 'silent classes' option to suppress the direct css output of the grid module #21

Closed matthew-andrews closed 10 years ago

matthew-andrews commented 10 years ago

The CSS Wizardry grid framework has an option for 'silent classes', which makes use of the SASS 'placeholder' selector.

https://github.com/csswizardry/csswizardry-grids#sass-silent-classes

This is achieved in that module with the following code:

$class-type:     unquote(".");

@if $use-silent-classes == true{
    $class-type: unquote("%");
}

And then every rule is implemented like this:

#{$class-type}grid{
    list-style:none;
    margin:0;
    padding:0;
    margin-left:-$gutter;
    @if $use-markup-fix != true{
        letter-spacing:-0.31em;
    }
}

This also means when this option is switched on unless one of the placeholder classes is one that has been extended, the compiled scss for the csswizardry-grid module is empty.

I believe there is currently no way to suppress the output of the origami grid module in the same way.

wheresrhys commented 10 years ago

We considered adding this option but I don't think there's really a use case for it unless we discard entirely the predefined grid classes.

I'm not sure exactly how the decision was made, but using predefined classes to apply the grid as been a given since before I started working on it. gzipped as a standalone file the css comes to 1,106 bytes so including the styles each time isn't a massive hit

matthew-andrews commented 10 years ago

That assumes that this library would only ever be used in a centrally controlled FT page. If someone were to make a FT-style web page without inheriting everything from it (ie. not using the 'wrapper' pattern) - for example microsites, the web app - then I think this would be an important feature to have.

wheresrhys commented 10 years ago

On the other hand if they had silent classes turned on then in general they couldn't in future choose to include any origami module which has o-grid as a dependency (or, at least, not without manually extending each selector that module and all its dependencies uses). So using silent classes can actually remove flexibility from products.

matthew-andrews commented 10 years ago

As a product developer I would be quite surprised if an origami component forced a responsive grid component on me, especially one that has the potential to interfere with my product's CSS.

I think that components would be a great use case to use the silent classes only as it would mean each components' css would only be the css precisely required for that component to work and nothing more (and wouldn't interfere with anything outside of its o-componentname- prefix).

matthew-andrews commented 10 years ago

(Apologies the link in the previous comment should have referred to #23, not this one)

wheresrhys commented 10 years ago

Just spoken to Andrew about this and concluded that it's best to keep a keen eye on how o-grid gets used by modules and products and decide whether @extends or predefined classes is best as the default way of using o-grid.

triblondon commented 10 years ago

I've thought about this some more:

Cons:

Pros:

Personally, I share Matt's view that we would do well to avoid having grid as a dependency in components - it ought really to be a direct dependency of a product. If that's the case, then I see no reason not to implement this, but with the proviso that any components that do use the grid MUST use the conventional classes, not silent ones (to comply with the rule that they must not change config in their dependencies).

matthew-andrews commented 10 years ago

I've just put together a quick gist here which should address your second and third points by always providing silent classes and giving product developers the option to switch off the non-silent classes - defaulting to on: http://sassmeister.com/gist/8297598.

For the first point I think it would be easy to make that same argument of "lots of things might need some of this, so let's just send everything" for any library. If we get into the habit of building pages that only ship with the resources they actually need at least the amount sent down the wire would scale with the amount used on any given page - rather than just continuously increase over time, even if in the most extreme cases the CSS is larger.

I think what I object to as a product developer is that components will force this grid system into my products. There are three ways I can think of to address this concern:-

- require components to use silent classes so that the fact they use any responsive grid CSS library is invisible to consumers of those components. - decide grid libraries are fundamental to the future of everything we make at the FT from here on out. Select a library to put on the A List (preferably an already established, open source one* - but if we must use this one at least be open that use of this grid library is mandatory). - decide which grid system to use is up to the product developer; mandate that components should not use a grid system and simply fill the space they have been allocated; and un-origami-ify and rename this grid library so that becomes the 'responsive article grid library'.

* I appreciate that no single standard way of doing grids has emerged, but I feel in these cases it would be better to back any horse than backing one that isn't even in the race.

wheresrhys commented 10 years ago

If o-grid was silent by default and non-silent was turn-onable, but not turn-offable I think that would prevent module breakages. The non-turn-offability would have to be by convention rather than implementation (I've experimented with mixins to try and enforce this, but as you'd need to import grid in order to get the mixin, then reimport to output the styles it's a bit of a mess).

This would be an exception to the rule that modules should not override their dependencies' settings. It seems a reasonable class of exceptions to add to the specs though - turning on an optional feature which has no effect on default features (unexpected bugs notwithstanding)

As regards to whether o-grid should be the FT grid I'm not sure if anyone really knows what the requirements for a grid are and what features should be mandatory across products (@triblondon - question for design this week?). There are some useful utilities in o-grid (definition of standard breakpoints, mixins for using them, ...) which, depending on what's mandatory across products, many components will want to use even if not laid out using the grid, so we should either:

triblondon commented 10 years ago

We can't have the non-silent version off by default, otherwise when you request the grid via the build service, you'd get nothing.

I'm persuaded that we should offer this. A product developer could be legitimately irritated if they requested a gallery component and with it they got a whole grid as well.

I am happy with the following solution:

The implications of this are that if a product developer uses the build service, they will always get the non-silent classes, but it seems like fussy product developers are likely to roll their own build anyway. In light of the proposed improvement to responsive selectors, it seems like we'll need a quite different set of mixins for doing silent:

Non-silent (maybe, see #23):

<div class="o-grid-col" data-o-grid-sizing="5,S12,XL3">

Silent:

<div class='myelement'>
.myelement {
  @extend %o-grid-col-d5;
  @extend %o-grid-col-s12;
  @extend %o-grid-col-xl3;
}
wheresrhys commented 10 years ago

We can't have the non-silent version off by default, otherwise when you request the grid via the build service, you'd get nothing.

If we had a naming convention for the variable to make a module's sass silent

$o-modulename-is-silent: true !default; 
// or we may have to use $o-modulename-isnt-silent: false !default; 
// I have a feeling sass won't let you override a default value with 'false'

Then build service could prepend the following to the sass before building (ensuring it catches all sub-dependencies should be easy enough)

$o-module1-is-silent: false;
...
$o-moduleN-is-silent: false;

This would have zero effect on modules that don't implement the silent class pattern and unsilence all modules that do implement it.

triblondon commented 10 years ago

Let's go with noisy by default for the moment, but use this pattern, and I'll add that to the build service backlog.

triblondon commented 10 years ago

Added as https://redmine.labs.ft.com/issues/19237 (we're tracking the build service in redmine). Also noted in that issue that ideally, the build service would only disable silent output on the top level of the dependency tree. If you requested moduleA and moduleA depended on moduleB, but you haven't explicitly requested moduleB, there's no need for B to be noisy. You could always turn noisyness on by adding B to your top level request.

triblondon commented 10 years ago

The Build service now does this. Is this supported in grid yet?

wheresrhys commented 10 years ago

Neat!

https://github.com/Financial-Times/o-grid-issues/issues?milestone=6&page=1&state=open are now all done in branches but not thoroughly tested.

Could you confirm whether you're happy with https://github.com/Financial-Times/ft-origami/issues/78#issuecomment-32178736 as I mistakenly branched to implement the new features from the branch that integrates the useragent changes, so would like to get the whole useragent thing decided before starting to release a gridv2

wheresrhys commented 10 years ago

implemented in branch v2