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
Comment | File | Size | Author |
---|---|---|---|
#15 | remove-bartik-ids-2347751-15.patch | 34.27 KB | DickJohnson |
#5 | interdiff.txt | 9.17 KB | rteijeiro |
#5 | remove-bartik-ids-2347751-5.patch | 30.25 KB | rteijeiro |
Comments
Comment #1
rvilarI'm starting to work on that
Comment #2
rvilarAttached is the patch that removes ids from bartik.
Comment #5
rteijeiro CreditAttribution: rteijeiro commentedThis 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.
Comment #7
sqndr CreditAttribution: sqndr commentedComment #8
sqndr CreditAttribution: sqndr commented@rteijeiro: Any updates on this issue?
Comment #9
emma.mariaPostponed 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.
Comment #10
DickJohnson CreditAttribution: DickJohnson commentedNow 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?
Comment #11
lauriiiLets unpostpone because no reason for postponing this anymore
Comment #12
DickJohnson CreditAttribution: DickJohnson commentedMy 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".
Comment #13
lauriiiBoth 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.
Comment #14
DickJohnson CreditAttribution: DickJohnson commentedAfter 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.
Comment #15
DickJohnson CreditAttribution: DickJohnson commentedWorked a bit on this. Nowhere near to ready but putting it to needs review to see what testbot has to say.
Comment #16
emma.mariaI 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.
Comment #17
emma.mariaOK 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 :)
Comment #18
DickJohnson CreditAttribution: DickJohnson commentedI 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.
Comment #19
LewisNyman CreditAttribution: LewisNyman commentedIt 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:
Comment #20
emma.mariaI 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 :)
Comment #21
idebr CreditAttribution: idebr commentedThe 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.
Comment #22
DickJohnson CreditAttribution: DickJohnson commentedWe've already started to work on this per component. An example can be seen there: #2398471: Clean up the "footer" component in Bartik
Comment #23
idebr CreditAttribution: idebr commentedOh 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.