Issue 6061 - Added important to utilities

Issue 6061 - Added important to utilities

[juliandoucette, 2017-12-01 15:32:18]

Detail: This change will resolve 6061. The separating and re-ordering of
utilities can happen in a separate issue.

[juliandoucette, 2017-12-01 15:33:12]

Detail/TOL: Perhaps a utility should be a standalone modifier (not to be
extended elsewhere) and utilities that are commonly extended should be base.scss
classes? 

If so, I think we can address that separately.

[ire, 2017-12-04 08:25:23]

On 2017/12/01 15:33:12, juliandoucette wrote:
> Detail/TOL: Perhaps a utility should be a standalone modifier (not to be
> extended elsewhere) and utilities that are commonly extended should be
base.scss
> classes? 
> 
> If so, I think we can address that separately.

I think you may be right, because I don't think everything in this utilities
file should actually have an !important on it. In fact, the only rules I think
need them *right now* are the `.unstyled` classes, because we know that those
are overriding styles set by `.content`. The `.container`, for example, probably
should not have `!important` on it. My inclination would be to only apply
`!important` where we see it's needed, rather than to everything at first.

[juliandoucette, 2017-12-04 13:12:59]

On 2017/12/04 08:25:23, ire wrote:
> I think you may be right, because I don't think everything in this utilities
> file should actually have an !important on it. In fact, the only rules I think
> need them *right now* are the `.unstyled` classes, because we know that those
> are overriding styles set by `.content`. The `.container`, for example,
probably
> should not have `!important` on it. My inclination would be to only apply
> `!important` where we see it's needed, rather than to everything at first.

Ack. Updated.

[ire, 2017-12-05 09:30:13]

One NIT/Question

https://codereview.adblockplus.org/29626562/diff/29629915/static/scss/_utilit...
File static/scss/_utilities.scss (right):

https://codereview.adblockplus.org/29626562/diff/29629915/static/scss/_utilit...
static/scss/_utilities.scss:114: list-style: none !important;
NIT: Is there a reason you removed the `list-style: none` style to not only be
applied to lists?

I don't actually know if we (eyeo) have a preference (coding style wise) for how
to handle cases like this. I..e is it better to apply all the styles to the one
class, or only target it to where it's needed, in this case lists. From the way
we handled the resets, my impression was that we prefer the latter. I have a
slight personal preference for the latter as well, but I recognise that the
former is better with file size.

[juliandoucette, 2017-12-06 14:33:34]

On 2017/12/05 09:30:13, ire wrote:
> NIT: Is there a reason you removed the `list-style: none` style to not only be
> applied to lists?

- file size
- fewwer selectors
- "Avoid qualifying ID and class names with type selectors."
(https://google.github.io/styleguide/htmlcssguide.html#Type_Selectors)

> I don't actually know if we (eyeo) have a preference (coding style wise) for
how
> to handle cases like this. I..e is it better to apply all the styles to the
one
> class, or only target it to where it's needed, in this case lists. From the
way
> we handled the resets, my impression was that we prefer the latter. I have a
> slight personal preference for the latter as well, but I recognise that the
> former is better with file size.

This was not opinionated. I just took the hint from the styleguide + thought
there were minor pluses (file size, fewer selectors) and no negatives
(performance?) that I know about.

[ire, 2017-12-06 14:56:14]

On 2017/12/06 14:33:34, juliandoucette wrote:
> On 2017/12/05 09:30:13, ire wrote:
> > NIT: Is there a reason you removed the `list-style: none` style to not only
be
> > applied to lists?
> 
> - file size
> - fewwer selectors
> - "Avoid qualifying ID and class names with type selectors."
> (https://google.github.io/styleguide/htmlcssguide.html#Type_Selectors)

Good points.

> > I don't actually know if we (eyeo) have a preference (coding style wise) for
> how
> > to handle cases like this. I..e is it better to apply all the styles to the
> one
> > class, or only target it to where it's needed, in this case lists. From the
> way
> > we handled the resets, my impression was that we prefer the latter. I have a
> > slight personal preference for the latter as well, but I recognise that the
> > former is better with file size.
> 
> This was not opinionated. I just took the hint from the styleguide + thought
> there were minor pluses (file size, fewer selectors) and no negatives
> (performance?) that I know about.

I'm not aware of any performance negatives. 

LGTM

[juliandoucette, 2017-12-06 15:41:06]

Woo!

[juliandoucette, 2017-12-06 15:42:42]

https://hg.adblockplus.org/website-defaults/rev/6de544d8497c