Financial-Times / ft-origami

The Old Origami Website, do not use
https://github.com/Financial-Times/origami
76 stars 12 forks source link

Consider allowing long selectors for eg tables #243

Closed triblondon closed 10 years ago

triblondon commented 10 years ago

In https://github.com/Financial-Times/o-techdocs/pull/20 we were forced to write less good CSS (which will incorrectly style tables that do not have a class attached simply because they are inside a table that does) because otherwise it would fail linting.

Should we consider dropping this lint rule and dealing with it by peer review?

kaelig commented 10 years ago

Thanks for including me in the discussion. I agree, good CSS takes precedence over linting.

Would this be a good compromise?

.o-techdocs-content {
  table {
    // Unquote direct children selectors:
    // allows a deeper nesting than authorised by the linter
    > #{unquote("thead > tr > td")},
    > #{unquote("tbody > tr > td")},
    > #{unquote("thead > tr > th")},
    > #{unquote("tbody > tr > th")} {
      // properties
    }
  }
}
triblondon commented 10 years ago

Let's go with Kaelig's solution, which still encourages shorter selectors.

roland-vachter commented 10 years ago

Here is another use case: o-livefyre-comment-client, which is a module that integrates Livefyre's widget. We customize it for FT, which means a lot of overrides of default Livefyre CSS. To do this, we have to use a higher precedence of what Livefyre is using, which forces us over nest level 3. I would also be happy to delete this rule.

matthew-andrews commented 10 years ago

I'm a bit concerned about having #{unquote(...)} will pollute all our CSS files with a lot of bloat that doesn't add value... It affects their grep-ability, adds a lot of noise and feels like it will punish the wrong devs.

I think we should still encourage short selectors but perhaps do it via peer review (or perhaps this rule is OK but just loosened a bit - increasing the maximum depth perhaps?)

roland-vachter commented 10 years ago

I agree Matthew.

An example from my use-case: I should override this rule:

.fyre-editor .fyre-editor-toolbar .goog-toolbar > div.fyre-button-left .fyre-button-left-outer-box .fyre-button-left-inner-box

by using just classes. The above reached depth 6, so we need depth 7 to override. I think the max I reached was 8. One can argue that it's wrong, but in this case this is the only way.

kaelig commented 10 years ago

At the Guardian we excluded a few files from the maximum selector depth linter: third party overrides and styling of content that came from the CMS. Any chance we can do that too?

kaelig commented 10 years ago

I should mention that scss-lint should support linter switching at a file level, à la jslint/jshint.

Feel free to contribute to the discussion here: https://github.com/causes/scss-lint/issues/150