Follow-up to #2422377: elements.css css lint fix

Problem/Motivation

fix css lint issues:
Line Column Message
53 1 Heading (h1) has already been defined.
57 1 Heading (h2) has already been defined.
61 1 Heading (h3) has already been defined.
65 1 Heading (h4) has already been defined.
69 1 Heading (h5) has already been defined.
73 1 Heading (h6) has already been defined.
undefined undefined You have 2 h1s, 2 h2s, 2 h3s, 2 h4s, 2 h5s, 2 h6s defined in this stylesheet.

Proposed resolution

rewrite the h1-h6 selectors

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
csslint-elements-css-bartik.diff1.14 KBmortendk

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Needs work

This seems a bit wasteful to me... maybe don't believe all csslint's recommendations? Judgement call on this one needed.

With the duplicate properties, there is more chance of inconsistencies.

mortendk’s picture

mortendk commented

Im a fan of playing hardball here, and follow hard csslints recommendations.
* It makes it clear for everybody whats expected.
* we dont end up with "messy css", where we try to be extra smart on sharing properties & then have repeating selectors.

If we dont go with the hardcore way, we should create our own version of the lint tool, so we can automate it.

Im for going all hardcore on this, it also remove any kind of bikeshedding ;)

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs work » Needs review

We are able to pick and choose which CSSLint rules we follow, see the patch in #2222049: Add a .csslintrc file that's in line with our CSS standards

The question is less "do we agree with this rule" and more "is this rule fit with our CSS standards".

My understanding of the relevant area of SMACSS is not to duplicate the CSS selectors in multiple places, so we know we have one place to look to see all the CSS that affects that element. In other situations we've been removing the files that have long lists of selectors with one or two sahred properties in so we can maybe the code easier to find for each element. eg. #2398447: Remove the "typography" CSS file in Bartik

Happy to go into this in more detail if people want to, but my instinct is to follow the direction of the csslint rule here.

mortendk’s picture

mortendk commented

im +1 for the "csslint is the law"
If we dont create our own ruleset, or we dont follow csslint strict, then we end up with potential loopholes. + it would be sweet to have a "lint trap" on all submitted css, its gonna make it a pita to add that with potential loopholes.
yes im a css-nazi ;)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Just recommending against this because duplication of the properties on the grouping (all header elements), makes it easy to create mistakes when changing things. #maintenance.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Actually I've got a change of heart here.

  • It helps be consistent with CSS lint
  • The margin should probably be not based on the font-size anyways, so that should change for better Vertical Rhythem later, so they will or could be different.
  • And the inherit change is just one property so feels a bit wasteful at that point.
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 2da7950 on 8.0.x 
    Issue #2422381 by mortendk, joelpittet, LewisNyman: elements.css css...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.