Financial-Times / o-grid

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

Not all classes are namespaced #23

Closed matthew-andrews closed 10 years ago

matthew-andrews commented 10 years ago

If you as a product developer choose to use classes with pipes in the following form to mean something specific to your project, the CSS from the responsive grid will interfere with your CSS:

.myproduct|s0|

Origami spec seems to suggest all classes within an origami module should be namespaced - the following 'partial classes' in the responsive grid module are not namespaced:-

Plus any of the following where ${prefix} is one of s, m, l or xl

It seems like if a product developer wanted to use a similar pipe pattern in their css classes they would not be able to use the responsive grid (or any component that depended on the responsive grid) without risking the two css codebases interfering with each other.

To fix this each of the 'partial classes' could be namespaced but the result may be quite ugly:-

o-grid-col|o-grid-3|o-grid-m12|o-grid-s12| 

Therefore I suggest removing this pipe syntax and providing all the grid functionality through o-grid- prefixed classes.

triblondon commented 10 years ago

This is a duplicate of #7. I think your proposal is overzealous, Matt. I do share the concern over leaked selectors though, and it's come up before. I proposed this kind of thing in #7:

[class^="o-grid"][class*="|d6|"] {
  width: ...;
}

Rhys put some effort into trying it out and had specificity concerns.

matthew-andrews commented 10 years ago

How is suggesting using normal CSS selectors like every other CSS grid library I've seen overzealous?

This pipe syntax seems like a very non-standard way of defining CSS for a project that is all about standards.

This would be easy to fix now and really hard once this is a dependency of half the things we build from now on. Of all the components to get right I would put this one near the top.

triblondon commented 10 years ago

As far as I'm aware, Foundation uses this approach and was what inspired this syntax, and Font Awesome also uses class name fragment matching. So there's certainly precedent for it.

There is absolutely no commonly accepted way of doing grids - Harry's lap- and palm- prefixes, for example, are equally esoteric, and I think semantically nonsensical. But I see how you'd come up with them, and I don't think the world would end if we did that, and I don't think the world would end if we used classname parsing either.

My solution appears to resolve your concern, and you haven't said why you don't like it except that it's not 'normal'. I say it is, and have cited two examples in very popular use.

matthew-andrews commented 10 years ago

Foundation uses this approach and was what inspired this syntax

No it doesn't. For grids it uses syntax like this:-

small-4 large-4 columns

Font Awesome also uses class name fragment matching.

It does indeed - but all the classes it matches against are prefixed with icon-, which makes the leakages a little safer.

https://github.com/FortAwesome/Font-Awesome/search?l=scss&q=%5Bclass&ref=cmdform

My solution appears to resolve your concern, and you haven't said why you don't like it except that it's not 'normal'. I say it is, and have cited two examples in very popular use.

Unfortunately it doesn't stop the leaking and it also increases the specificity: http://codepen.io/matthew-andrews/pen/IAygv.

triblondon commented 10 years ago

Maybe they changed Foundation. It used single classes when I last looked at it.

I didn't appreciate that the starts-with and the contains match might match different classes, which makes sense and definitely makes my solution less appealing (and the specificity issue is already well discussed in #7), though I'm confused by the fact that you're happy with Font Awesome doing exactly the same thing.

Nevertheless, I'm persuaded of the argument for switching away from parsing fragments of classes, much as I dislike the idea of having lots of separate name-spaced classes cluttering up the DOM, we should probably do this:

class="o-grid-col o-grid-6 o-grid-s12 o-grid-l3"

@wheresrhys, any thoughts?

wheresrhys commented 10 years ago

I got the attribute selector idea from here http://24ways.org/2012/a-harder-working-class/ and a few other grids make limited use of attribute selectors too, but generally where the fragments matched are prefixed with something.

The pipe syntax was chosen because of its very non-standarness (although it does comply with w3 css & html standards) as it reduces the risk of it clashing with classes added by other devs (it's not so very different to using the 'o-' prefix which also relies on the underlying assumption that other devs are unlikely - but not guaranteed - to use the o- prefix). | is invalid within a class selector (only valid in an attribute selector) so any other classes containing | would most likely be added by modules that also use the unusual practice of piped classes.

A lot of the above was based on the assumption that the grid would be especially privileged as something that would be used by most components (not a God module... more of a saint perhaps) and therefore would own | within FT products... but if that assumption was erroneous then there's a stronger case for changing to separate classes.

One final suggestion - again kinda non-standard, but in scopeless CSS non-standard is often better - something like this preserves namespacing, conciseness and expresiveness

<div class="o-grid-col5" data-o-grid-responsive="|S12|XL3|">

And we don't need to worry about ie7&8 as they fallback to the fixed grid anyway.

matthew-andrews commented 10 years ago

The pipe syntax was chosen [..] as it reduces the risk of it clashing with classes added by other devs

If someone else has the same idea the opposite may be true.

it's not so very different to using the 'o-' prefix which also relies on the underlying assumption that other devs are unlikely - but not guaranteed - to use the o- prefix

But this library relies on ownership of the o-grid prefix as well. If you're concerned that the o- prefix won't offer robust enough protection against other libraries then that's something we need to discuss and fix in the origami spec.

but in scopeless CSS non-standard is often better

Could you provide more details on this? I'd be really interested in learning more about this as it's quite different to the advice I've seen before.

something like this preserves namespacing, conciseness and expresiveness <div class="o-grid-col5" data-o-grid-responsive="|S12|XL3|">

I think there is much greater potential to cut down the amount of code we need to write by using an existing established, open source grid library than developing our own that has slightly more concise than average syntax. EDIT: This wasn't helpful. The proposal @wheresrhys suggests would solve the issue of leakages/namespacing.

triblondon commented 10 years ago

Matt, note that you haven't commented on the validity of the data attribute solution, and we are already addressing the issue of the grid module's existence in a separate issue. Let's keep this thread on topic.

Personally, I think I'm completely persuaded that we need to change what we have, and Rhys's data-attribute solution seems neat.

triblondon commented 10 years ago

I just spoke to Matt about this, and we agree that Rhys's suggestion sounds good, with the slight worry that it seems 'surprising' (Matt) and 'innovative' (Andrew). Basically has anyone else done this and if not, why not!?

So, subject to that being not a concern, this is the syntax we all seem to be happy with:

<div class="o-grid-col5" data-o-grid-responsive="|S12|XL3|">

I wonder whether it would be better to put the default in the data attribute as well, which would mean you wouldn't have to define classes for all twelve columns, and you could also shorten the data attribute name a bit! Finally, since we're in a data attribute, the pipe character seems esoteric and I'd suggest we move to a comma (unless you still need the prefix and suffix delimiter as well):

<div class="o-grid-col" data-o-grid-sizing="5,S12,XL3">
matthew-andrews commented 10 years ago

You're right. Apologies to all for an unhelpful comment. I have edited the above to reflect this. I think a major benefit of Rhys' suggestion is that it should be relatively simple to implement on top of the existing grid implementation.

kavanagh commented 10 years ago
<div class="o-grid-col" data-o-grid-sizing="5,S12,XL3">

Rather than comma separated why not have it space separated and use the tilde attribute selector E[foo~="bar"]:

E[foo~="bar"] an E element whose "foo" attribute value is a list of whitespace-separated values, one of which is exactly equal to "bar"

So instead we have

<div class="o-grid-col" data-o-grid-sizing="5 S12 XL3">
triblondon commented 10 years ago

Love it.

kavanagh commented 10 years ago

Also can we assume an element is a grid column if it has a o-grid-sizing data attribute? If so, no need to identify it as such with a class?

wheresrhys commented 10 years ago

Even though the only reason I took this job was for the opportunities to use pipes in stylesheets, I do like those spaces

The reason I kept class="o-grid-col5" is because a non-responsive column is the most common use case and in that case the styles will all come from the class, as you'd expect it to be. The responsive stuff in the data attribute can be seen as optional overrides. Matter of taste which is best:

<div data-o-grid-sizing="5">
<div data-o-grid-sizing="5 S12 XL3">
<div class="o-grid-col5">
<div class="o-grid-col5" data-o-grid-overrides="S12 XL3">
robin-marr-ft commented 10 years ago

you could also try the dash separator |= and use

data-o-grid-sizing="5-S12-XL3"

Kinda shows that they're connected but different

On 7 January 2014 14:37, Luke Kavanagh notifications@github.com wrote:

Also can we assume an element is a grid column if it has a data-o-grid-sizing, therefore not needing to identify it as such with a class?

— Reply to this email directly or view it on GitHubhttps://github.com/Financial-Times/o-grid-issues/issues/23#issuecomment-31741939 .


This email was sent by a company owned by Pearson plc, registered office at 80 Strand, London WC2R 0RL. Registered in England and Wales with company number 53723.

triblondon commented 10 years ago

I much prefer sizing to overrides, even if we have the default size in the class. Also prefer spaces to dashes (see Robin's point but the connection is already expressed by the fact that they're in the same attribute value). I would consider adding 'span' to the class though to make it clear what the '5' is for (otherwise it reads as 'column 5'):

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

But, provided there's no performance issue to not having a class at all, and provided that you can make use of the silent extends technique to use classes if you want to, I very much like @kavanagh's suggestion:

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

That's really concise AND expressive. If it's also robust and performant, it's a winner for me.

matthew-andrews commented 10 years ago

Yes, this is neat.

Also the Chrome team seem to think css selector performance is a bit of a non-issue anyway so I think a couple of dozen attribute selectors ought not do much harm (although I can't verify this because they disabled the profiling tools):- https://code.google.com/p/chromium/issues/detail?id=265486

wheresrhys commented 10 years ago

Early on in developing the grid I profiled attribute selector performance on a huge page and the difference between class and attribute selectors was negligible. Should be even less of a worry if we're using the ~= matcher on an attribute reserved for grid styles.

triblondon commented 10 years ago

Right, go for it.

wheresrhys commented 10 years ago

OK, gonna close this issue and open a new one just for implementation of the the new syntax

I'll mark all enhancements agreed over the past few days for v2. If anyone has any other suggestions for v2 (e.g. features present in existing grid frameworks but missing from o-grid) please raise them now so they can get into v2