Problem/Motivation

The issue #2349591: Dropdown action list shows up as a simple list without JavaScript introduced a visual regression in the dropbutton:

Before regression:

After regression:

Proposed resolution

Fix the regression

Remaining tasks

  1. Write a patch
  2. Review

User interface changes

The collapsible dropbutton links can be clicked over the entire width of the button.

API changes

None

CommentFileSizeAuthor
#3 2430999-3.patch472 bytesidebr
#3 2430999-3-after-nojs.png124.75 KBidebr
#1 2430999-1.patch420 bytesidebr
#1 2430999-1.gif47.97 KBidebr
dropbutton-regression.gif70.06 KBidebr
dropbutton-before.gif57.83 KBidebr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

idebr’s picture

idebr CreditAttribution: idebr commented
Component: CSS » Seven theme
Issue summary: View changes
Status: Active » Needs review
Related issues: +#2349591: Dropdown action list shows up as a simple list without JavaScript
FileSize
2430999-1.gif47.97 KB
2430999-1.patch420 bytes

Attached patch fixes the visual regression. Screencap after:

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Needs work

Thanks for the patch. I thought we had a no-js class but we don't...

+++ b/core/themes/seven/css/components/dropbutton.component.css
@@ -65,6 +65,9 @@
+  .js .dropbutton li + li {
+    margin-left: 0;
+  }

Instead of undoing the CSS for JS, maybe we can only set the CSS on :not(.js)?

idebr’s picture

idebr CreditAttribution: idebr commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2430999-3-after-nojs.png124.75 KB
2430999-3.patch472 bytes

@LewisNyman Attached patch uses your suggestion, but I'm hesitant to use this pattern:
- It introduces styling specific for non-javascript users instead of using javascript for progressive enhancement
- It would be the first occurrence of this type of selector in HEAD

Personally I would prefer the approach in #1, but I'll leave the call up to you.

Screenshot with javascript disabled:

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Reviewed & tested by the community

The .no-js class is commonly used on the web, right? The :not(selector) is just a modern equivalent of this. I'd rather not override CSS and the SMACSS principles we've embraced in our coding standards are designed to avoid this kind of CSS. Setting to RTBC and we can at least get a committer's opinion on it.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

From the look of it, dropbutton.component.css has approximately 50 gazillion .js .something-something rules and zero ":not" rules. Therefore, I think #1 is probably best, though it might be worth a separate issue to discuss whether / how to introduce ":not" syntax.

Committed and pushed #1 to 8.0.x. Thanks!

  • webchick committed b79ba56 on 8.0.x 
    Issue #2430999 by idebr: Visual regression in dropbutton

Status: Fixed » Closed (fixed)

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