Problem/Motivation

Avoid using the id selector in CSS. There is no general benefit to using the 'id' attribute as a CSS hook, and it has serious downsides in the form of increased selector specificity.

- from the CSS architecture (for Drupal 8)

Using ID selectors as styling hooks are bad practice according to our CSS coding standards. We should use classes.

We have some id-s in page.html.twig and other .twig files such as #header, #content, #footer, etc... and some styling applied in the related css files.

Proposed resolution

We should switch from id's to classes both in template files and where they are mentioned in Bartik's CSS files.
The ids will be replaced per component, see #1342054: [META] Clean up templates and CSS

But pay attention to these exceptions stated in the Coding standards https://www.drupal.org/node/1887918/#closing

Remaining tasks

Write patch.
Code review.
Visual review - with screenshots to show Bartik does not break from this work.

User interface changes

None

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rvilar’s picture

Assigned: Unassigned » rvilar

I'm starting to work on that

rvilar’s picture

Status: Active » Needs review
FileSize
22.76 KB

Attached is the patch that removes ids from bartik.

Status: Needs review » Needs work

The last submitted patch, 2: 2347751-2-remove-bartik-ids.patch, failed testing.

Status: Needs work » Needs review
rteijeiro’s picture

Assigned: rvilar » Unassigned
FileSize
30.25 KB
9.17 KB

This patch still needs more work. I fixed a few remaining ids but it looks like it breaks frontpage styling. I'll discuss it with lewisnyman and continue working on it.

Status: Needs review » Needs work

The last submitted patch, 5: remove-bartik-ids-2347751-5.patch, failed testing.

sqndr’s picture

Issue summary: View changes
sqndr’s picture

@rteijeiro: Any updates on this issue?

emma.maria’s picture

Status: Needs work » Postponed

Postponed as we are first splitting the CSS files into a different file structure to follow D8 standards here #2375673: Split Bartik's CSS into SMACSS style components.
Any patches for this issue in the meantime will need a big manual reroll when #2375673: Split Bartik's CSS into SMACSS style components is committed.

DickJohnson’s picture

Now that CSS -> SMACSS issue mentioned in previous post by emma.maria is commited, I think that reorganizing this issue would be great. As pretty much every file on bartik is affected by this I'd say that this issue should work as parent and that we should create maybe 5 or 6 new issues so that 3-4 css-files would be changed in one. Right?

lauriii’s picture

Status: Postponed » Needs work

Lets unpostpone because no reason for postponing this anymore

DickJohnson’s picture

Assigned: Unassigned » DickJohnson

My opinion says it should be postponed until the template cleanup issue has been commited.

After investigating this for 30 minutes I think that the new issues should be created per template. As an example one could be something like "remove id:s from page.html.twig and make relevant changes to css".

lauriii’s picture

Both good ideas; lets wait till template coding standard issue has been commited and lets create separated issues for each template so we don't run into crazy bikesheds. Or if we do it might happen only in some issues without blocking others getting in.

DickJohnson’s picture

After investigating this a bit more, I'm starting to be pretty sure this must be handled once. Reason is that page.html.twig affects nearly all files anyways.

DickJohnson’s picture

Assigned: DickJohnson » Unassigned
Status: Needs work » Needs review
FileSize
34.27 KB

Worked a bit on this. Nowhere near to ready but putting it to needs review to see what testbot has to say.

emma.maria’s picture

I think we should stop this work here and have it as part of the CSS cleanup issues so everything is done in one go under each component. If we just do this task on it's own we are not really refactoring the code, taking out unneccessary parts etc we are just switching ID's for classes and we will end up doing extra work elsewhere too. Removing ID's is part of #1342054: [META] Clean up templates and CSS.

I am working on structuring the issues going forward from #1342054: [META] Clean up templates and CSS right now.

emma.maria’s picture

Issue summary: View changes

OK I 360 on my decision. I took a hard look at the CSS files and saw how many IDs are spread out all over the CSS files right now, this task appears to be template driven and cannot be split up into CSS component issues.

Important: We still need to go on and assess CSS rulesets that are in the wrong files or have over-specific selectors within the relevant CSS component issue. See META issue here for where that work is defined... #1342054: [META] Clean up templates and CSS.

Patch away :)

DickJohnson’s picture

I think that due keeping issues small we shouldn't actually remove the ID's now. Just add relevant classes to page.html.twig, start the CSS refactoring and remove the ID's when they are not being targetted from CSS anymore. After #1903048: Revise Bartik template indentation inline with best practices. has been commited the class adding issue should be relatively small.

LewisNyman’s picture

It feels like part of the problem here is that we have the ID 'components' in many different files, should we fix that instead of just replacing IDs with classes? It feels like we're going to have to go through all these selectors again later a move them around/rename them. This issue feels like a very superficial win, we're not fixing the underlying problem.

What if we had one issue for each component (eg. footer/header/page/etc) that fixes:

  1. The names of the classes
  2. The location of the CSS in files
  3. The selectors used
emma.maria’s picture

I agree with #19, if we aiming to componentize the code in Bartik, we should componentize the issues.

We do not want to increase the workload by first working on this issue which is a quick "win" and then going on to improve the same code again in further issues. Yes it will be more work organising and creating the component issues, but the original plan of Bartik was to keep the work small and focussed.

I am leading towards setting this issue to Closed ( Won't Fix) and including removing ID work as part of this #1342054: [META] Clean up templates and CSS section of work.

Can I have feedback please or I will go ahead with this soon :)

idebr’s picture

Assigned: Unassigned » idebr
Status: Needs review » Needs work
Issue tags: +Needs reroll

The problem with coding with css ids is their specificity. By splitting this patch into components, you will probably run into specificity issues where styling by id overrides the new classes. The fastest way forward is probably to do a quick find and replace to convert all ids to classes and then work on the components.

Anyway, patch no longer applies after #1903048: Revise Bartik template indentation inline with best practices. was committed. I'll work on a reroll and finishing up this issue.

DickJohnson’s picture

We've already started to work on this per component. An example can be seen there: #2398471: Clean up the "footer" component in Bartik

idebr’s picture

Issue summary: View changes
Status: Needs work » Closed (duplicate)
Issue tags: -Needs reroll

Oh okay, let's give that a try then :). I'll mark this one as "duplicate" to prevent confusion and update #1342054: [META] Clean up templates and CSS accordingly.