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
While I was dealing with #2560231: Replace a class in forum-list.html.twig with a BEM class. I noticed that all but the first cell of the forum list table use the .forum__[element] naming convention. The first cell has the class .forum-list__forum which differs from its siblings naming pattern.
Proposed resolution
Add the .forum__info class to Classy's template to replace .forum-list__forum . Check that no other CSS exists styles the forum name with the current selector in HEAD.
Remaining tasks
User interface changes
none
API changes
none
Data model changes
none
Beta phase evaluation
| Issue category | Task because it does not creates visual issues. |
|---|---|
| Issue priority | Not critical because the forum functions fine. |
| Unfrozen changes | Unfrozen because it only changes markup |
| Prioritized changes | The main goal of this issue is to make coherent the class names of a component in Core. |
| Disruption | Non-disruptive it fixes a DX issue. |
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | 2568457-29.patch | 615 bytes | lucassc |
| |||
| #25 | interdiff_24-25.txt | 444 bytes | Suresh Prabhu Parkala |
| #25 | 2568457-25.patch | 1.18 KB | Suresh Prabhu Parkala |
| #24 | 2568457-22.patch | 615 bytes | Nitin shrivastava |
| |||
| #11 | 2568457-11.patch | 1.05 KB | Vidushi Mehta |
Comments
Comment #2
thamasComment #3
thamasHere is the patch.
Comment #4
thamasComment #5
thamasI did not find any CSS that themes the forum with the current class of
.forum-list__forumanywhere in Core.Comment #6
csakiistvanIt works, thx
Comment #7
LewisNymanI'm not sure about this, because the this element doesn't contain the forum info. It contains the entire forum. Is that right? That's why it's described as a forum inside a forum-list
Comment #8
thamas@LewisNyman The element is the first cell in a table row in which all cells contain information about the same forum. The cells ar siblings in html and their content also related. The only difference that the first cell has some divs in it. These are the icon, the name and the discription of the forum which is information about the forum--this is the source of my name suggestion. Of course the other cells also contain info about the forum so if you have a better name suggestion feel free to add it.
Otherwise the
trandtableelements have different names also so we could have a longer discussion about if we have only one component with elements inside it or we have a component with other components in it.I'm sure that the current naming is not OK.
Comment #11
Vidushi Mehta CreditAttribution: Vidushi Mehta as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedAdding a patch
Comment #12
Vidushi Mehta CreditAttribution: Vidushi Mehta as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedComment #22
smustgrave CreditAttribution: smustgrave at Mobomo commentedclassy has been removed in D10 but this could apply to claro theme also which may need some work.
Comment #24
Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commentedreroll for 9.5.x
Comment #25
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedTried to fix the CCF issues of patch from #24. Please review!
Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedAlso tagging for change record.
Comment #28
smustgrave CreditAttribution: smustgrave at Mobomo commented+ * {@inheritDoc}Out of scope
Added a change record
Tagging for novice as the reroll should be simple.
Comment #29
lucasscreroll for 10.1.x
Comment #30
sahilgidwani CreditAttribution: sahilgidwani at Axelerant commentedI have reviewed and applied #29 patch and it works fine for me with drupal 10.1.x.
Comment #31
bnjmnmIf this change is going to happen, it should also happen with the other themes doing this: Olivero and Umami
I'd like to suggest changing this to "won't fix" If this landed when the issue was created in 2015 (Drupal 8 was still in beta) I'd be all for it. ~7 years in, the risks of disrupting an existing site with the classname change (however unlikely) still seems to outweigh the benefits of changing the class name many years later.
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commented@bnjmnm could this almost be a policy that we should avoid adding new classes to core templates? Will post to #frontend to get a theme API maintainer thought.
Comment #33
lauriiiI don't think this is actually a theme system issue so moving to the other component. I agree that benefits of the change would be minimal. Regardless of that, to me, it seems fine to fix this in the product themes (Claro, Olivero, and Umami). It's allowed by the BC policies of these themes. Also, based on the contrib search, it doesn't seem like this would break anything in contrib.
Comment #34
bnjmnmWe do want the flexiblity to change stuff in the product themes. Starterkit (and Stable/Stable9) exist to provide markup predictability for sites that need it so we have freedom in Claro/Olivero/etc. My reservations are specific to this specific use case because I'm not convinced there's any noticeable benefit to making such a change in 2023, yet there's a slight chance this being an unwelcome disruption to someone.
But ultimately, if another committer and/or FEFM thinks it fine and this lands, I'm fine with it. While it's true I'm expressing an opinion regarding where this issue goes, that opinion is positioned much closer to indifference than it is "strongly agree"