Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
I created #2400233: Add reusable heading classes to Bartik for Bartik to apply part of the existing work in this Seven issue #2028053: Add typographic styles, components, and utility classes. From looking at the CSS I realised that the classes "header-a", "header-b" etc were used instead of "heading-a", "heading-b" for the reusable heading classes. These are definitely heading classes and nothing to do with header, so the naming was a mistake that was never picked up on.
From Seven's elements.css
h1,
h2,
h3,
h4,
h5,
h6,
.header-a,
.header-b,
.header-c,
.header-d,
.header-e,
.header-f {
font-weight: bold;
margin: 10px 0;
}Documentation to back up my point https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements
Proposed resolution
To make the default themes consistent and provide a unified good example in core for contrib themes, we should amend the class names in Seven.
Please change all instances of "header-..." to "heading-..."
Remaining tasks
Write a patch.
Review the patch.
User interface changes
none
API changes
n/a
Beta phase evaluation
| Issue category | Task because Seven's UI is not broken because of this, it's just fixing the naming convention of the code. |
|---|---|
| Issue priority | Not critical because it's a code cleanup task but needed to keep Seven and Bartik consistent and a good example for contrib. |
| Unfrozen changes | Unfrozen because it only changes CSS |
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | interdiff-2402061-7-11.txt | 777 bytes | emma.maria |
| #11 | drupal8-reuse-heading-class-2402061-11.patch | 1 KB | emma.maria |
| #7 | interdiff.txt | 553 bytes | Katiemouse |
| #7 | 2402061-7-reusable-heading-classes.patch | 1.04 KB | Katiemouse |
Comments
Comment #1
emma.mariaComment #2
emma.mariaComment #3
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedChanged all instances of "header-..." to "heading-..." in attached patch.
Comment #4
emma.mariaComment #5
emma.mariaThe patch applies cleanly with no errors and contains all of the work required and there are no other instances of
.header-in Seven. Thanks @er.pushpinderrana!Comment #6
alexpottConsidering these classes are not used perhaps a comment is order to explain why we have them - so we won't remove them in some clean up.
Comment #7
Katiemouse CreditAttribution: Katiemouse commentedHi, I am in high school and this is my first contribution to Drupal core. I have tried to do the suggested comment above. Attached is the patch and interdiff. :)
Comment #8
gvsoHi @Katiemouse! Great to hear that, let's wait for @alexpott. Did you notice that #2400233 needs the same? So, that can be your second contribution :)
Comment #9
Katiemouse CreditAttribution: Katiemouse commentedThank you :) I'm working on it now :)
Comment #10
LewisNymanI am being picky but I would like to change the comment here. I think they will be used in the future so maybe we should just define their purpose instead of just that they aren't used.
How about:
"Reusable heading classes are included to help modules change the styling of headings on a page without affecting accessibility."
I don't think we need to link to the other issue.
Also, I've never seen us place a comment between selectors, can we put it above the h1, h2, h3 selectors please? Thanks!
Comment #11
emma.mariaThis issue hasn't moved in a while.
Here's a new patch and interdiff. I have changed the comment text as per #10 and moved the comment above the selectors with a space above as per the CSS formatting standards.
Comment #12
LewisNymanPerfect, thank you.
Comment #15
emma.mariaPutting the status back as testbot had a wobbly.
Comment #16
alexpottCSS changes are not subject to beta evaluation. Committed 7500715 and pushed to 8.0.x. Thanks!