Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanr’s picture

Status: Active » Needs review
FileSize
1.2 KB

This patch fixes it by making the CSS selectors operate on ul.menu rather than any ul in the footer. I think this is better anyway, since you may not want all footer ul's to be styled that way (i.e. if you put a regular old ul in a custom block).

seanr’s picture

Screenshot of it fixed.

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community

Ok... patch looks good. Reviewed with Dreditor and simplytest.me.

Although, I still hate how the text is dark and there's an underline when you hover over a link. But that's out of scope. We should create a follow up issue for that.

jibran’s picture

+1 for RTBC. Working fine on RTL as well.

seanr’s picture

The underline is defined up in #footer-wrapper a:hover which makes it hard to fix without breaking other expected behavior. The other issue is a CSS conflict within contextual.theme.css, line 105 (default state) uses !important, but line 118 (hover state) does not, and so never overrides it.

seanr’s picture

LewisNyman’s picture

Code looks good and patch applies, I also noticed that the font size becomes too small, but that's a wider problem with the contextual link CSS.

RTBC++

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Did you ever knoooowww that you're my heeerrrrooooo...

Seriously, this bug has been ticking me off for months! :D

Committed and pushed to 8.x. Thanks!

echoz’s picture

Status: Fixed » Needs review
FileSize
1.24 KB

I see no reason why the committed patch from #1 qualified the .menu class with the ul tag (ul.menu). This patch replaces those instances with just .menu and in my testing works the same.

markhalliwell’s picture

Status: Needs review » Fixed

Thanks for the patch. This issue has already been committed. Please open a new issue to discuss concerns with an existing issue that has already been committed.

I will note the patch in #1 that was committed is suitable and acceptable CSS. Given that menus in core are generated as <ul/> lists, I see no real harm in this. Bartik does not override the default template to provide different markup like other contrib themes do. The reason for your discomfort with this patch is to do with CSS specificity, yes?

While yes, it is generally preferable to do simpler selectors whenever possible, Bartik is not a theme that provides a lot of complex inheritance or specificity issues. At this point, it is merely personal preference in how this gets implemented. You will notice that the original selector in the patch was #footer ul li. It is a very logical step to simply append the class to already existing code. In issues like these, sometimes a simple fix is just that: simple.

If you really feel strongly about having this minor detail corrected, I'd be happy to review a new issue with the patch provided there.

echoz’s picture

Created #2057317: cleanup recently patched qualified selector css

It is not just a "personal preference" that is "generally preferable", it is in our coding standards and IMO not "suitable and acceptable CSS" to newly add to D8.

webchick’s picture

FWIW, I'm fine with things like this being handled in the same (re-opened) issue, when the original patch was so small. Since folks are already subscribed, it's easier to notify all the people involved about the issue, and get their collaboration in fixing.

But as long as an issue's already spun up, I guess we can fix it over there.

echoz’s picture

@webchick AGREED about re-open process, thank you!

markhalliwell’s picture

Sorry, I can never tell when thing should be opened in a new issue or when it's fine to continue in an already committed issue. Is there a policy for this? If not, I think we should have one. It's really confusing and can make some of us come off "mean" when it's not intended so.

I also apologize for not being aware of the new CSS coding standards for D8. I have reviewed your patch in light of that.

echoz’s picture

@Mark Carver yeah, I don't know if there is a policy for this, in fact I've seen different opinions :-)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

added after screenshot