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.
In the recently committed patch in #1919658: Footer link styling overrides contextual links the fix was to add class .menu in a handful of #footer rulesets, but it qualified the .menu class with the ul tag (ul.menu). We still have plenty of subpar css in core but adding css that goes against our coding standards seems not ok. This patch simply replaces those instances from the previous patch of ul.menu with just .menu
Comment | File | Size | Author |
---|---|---|---|
remove-unneeded-selector.patch | 1.24 KB | echoz | |
Comments
Comment #1
markhalliwellRemove "li".
Comment #2
echoz CreditAttribution: echoz commentedI was going with the policy that a patch should not include other cleanup. There's actually another ul.menu at line 290 that I was tempted with but didn't touch. These being in the same ruleset, maybe, but I find 10 instances li.someclass in this file, and also instances of "li a" that could probably loose the li. I'm torn but I would say anything beyond this patch is unrelated Bartik cleanup.
I appreciate the comment and actually started out to rewrite the patch, but ended up not.
Comment #3
markhalliwellI agree that the patch shouldn't touch any other lines (if this is all we're going to change), but this patch is already modifying these lines. The inheritance (from the other lines) you mention shouldn't be a problem because the classes are preceded with an ID:
#footer
. Without actually testing it manually, my gut is to say it should be safe to remove "li" from these lines since there's an ID parent.Like you I'm torn because if we're going to go with the CSS standards, we should adhere to these standards whenever possible. Granted I didn't know they existed (too many pages pop up to keep tabs on, IMHO), but now that I do I think we can either do one of two things here:
On a side note, @mortendk and I have been discussing Bartik's cleanup for awhile. There is a ton of things that do need to get refactored. I can't remember off the top of my head if there's an issue for that yet or not. Personally, and considering this issue is relatively new, I think we should maybe go with the latter option and go ahead with cleaning up Bartik. As it stands, Bartik's CSS doesn't adhere to the new standards at all, so we're going to run into these type of back and forth opinions until it gets resolved. I propose we do that here. Thoughts?
Comment #4
echoz CreditAttribution: echoz commentedWe've been chipping away at this all year. #1921610: [Meta] Architect our CSS
I'd say move this patch through, it's point being correcting adding NEW qualified selectors, or drop it and let it get cleaned up with the rest of core themes. 2. would surely be a duplicate of an existing issue.
Edit: I didn't intentionally change any tags
Comment #5
markhalliwellNo worries, tags have been acting weirdly and keep getting eaten.
Alright then let's get this patch through then. I'll RTBC as is. I'm curious though, you say if we did 2 that this would be a dup of an existing issue. I can't seem to find one (for Bartik specifically), but may just be out of the CSS loop.
Comment #6
echoz CreditAttribution: echoz commented@Mark Carver, Some history in #1854344: [meta] Goals for core themes: Make Stark as semantic as possible; Make Bartik a better prospective base theme that was marked a duplicate of #1980004: [meta] Creating Dream Markup
I believe it would be covered over there.
Comment #7
markhalliwellYes, I'm very well aware of #1980004: [meta] Creating Dream Markup as it's part of the Twig cleanup (which I'm more involved with). That meta has more to do with the underlying markup though, they usually don't change the CSS. The original meta you mentioned (#1921610: [Meta] Architect our CSS) is what I was referring to. I didn't see anything in that issue or sub-issues referencing cleaning up Bartik's CSS. I was just saying that it seems we should make this issue a sub-issue of that meta to try and clean up Bartik's CSS (seeing as there is no issue for this currently).
Comment #8
echoz CreditAttribution: echoz commentedI believe it would follow the process of building components #1995272: [Meta] Refactor module CSS files inline with our CSS standards for the same reason we are not jumping on cleaning up Seven's (and core module's) existing selectors. The ambitious plan is to rewrite the css rather than just shorten selectors. For instance, why change "ul.menu li a" now when we want to change it to .menu__list including where the classes are generated from. If this doesn't get done, we could certainly do the easier cleanup of removing extraneous selectors.
Comment #9
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.