cleanup recently patched qualified selector css [#2057317]

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

markhalliwell

English

CreditAttribution: markhalliwell commented 4 August 2013 at 21:38

Status:

Needs review

» Needs work

+++ b/core/themes/bartik/css/style.css
@@ -1047,37 +1047,37 @@ ul.links {
+#footer .menu li.first a {
...
+[dir="rtl"] #footer .menu li.first a {
...
+[dir="rtl"] #footer .menu li.last a {

Remove "li".

Comment #2

echoz CreditAttribution: echoz commented 4 August 2013 at 22:34

Status:

Needs work

» Needs review

I 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

markhalliwell

English

CreditAttribution: markhalliwell commented 4 August 2013 at 23:08

Status:	Needs review	» Needs work
Issue tags:		+Bartik, +CSS cleanup, +css coding standards

I 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:

Use this issue for simply patching this one thing (including removing the li, if possible).
Increase the scope of this issue to cleanup Bartik's entire CSS and adhere to these new D8 CSS coding standards.

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 commented 5 August 2013 at 00:29

Issue tags:

-CSS cleanup

We'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

markhalliwell

English

CreditAttribution: markhalliwell commented 5 August 2013 at 01:28

Status:	Needs work	» Reviewed & tested by the community
Issue tags:		+CSS cleanup

No 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 5 August 2013 at 03:59

@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

markhalliwell

English

CreditAttribution: markhalliwell commented 5 August 2013 at 04:21

Yes, 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 commented 5 August 2013 at 05:53

I 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.