The list of actions has a bad fallback when JavaScript is turned off. The functionality is still working, but there are no graphical clue for the user that it's possible to trigger the actions. I think this is bad usability.

Remaining tasks

Check the latest patch doesn't affect dropbuttons in the following themes:
1. Seven
2. Bartik
3. Classy

A good selections of pages to test are:
1. /admin/content
2. /node/add/article
3. /admin/structure/views/view/content

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

Issue tags: +Usability, +JavaScript, +frontend, +CSS

Ah I get it, these should look like buttons when JS is disabled right? I think we just need to add .js in front of some of the CSS?

juho.lehmonen’s picture

Status: Active » Needs review
FileSize
5.48 KB
1.08 KB

Here's a patch for a nicer no-js dropbutton fallback. I added .js before the background and border overrides, so those don't happen when javascript is disabled.

I also added some alignment and margin styles to align the buttons better without javascript. Any opinions on this? I'm pretty sure there's no risk of this bleeding through, but I suppose there might be a cleaner way of doing this.

juho.lehmonen’s picture

Disregard the previous patch, I forgot to reset the default ul styles for the fallback (padding, margin and list-style-type). This one should work.

Outi’s picture

I tested and it looks good for the publishing buttons (the screenshot A).

I just checked how the buttons would be on a view editing page (admin/structure/views/view/content), and there are some problems with the patch (screenshot B) and without the patch (screenshot C), but I don't know if this should be added to this issue or not, because the patch #3 clearly fixes the original problem.

juho.lehmonen’s picture

It seems to be a bug with disabled javascript in Views UI. The patch didn't really make the situation worse, it's just broken in a different way now.

Is there already an issue about no-js usability in Views UI? I could take a look at fixing this if no one else is currently working on it.

Outi’s picture

No, it didn't make it worse, the Views UI without JS just doesn't seem to be fixed. I think it should be fixed in another issue and consider this issue fixed with the patch #3.

LewisNyman’s picture

Status: Needs review » Needs work

No, it didn't make it worse, the Views UI without JS just doesn't seem to be fixed. I think it should be fixed in another issue and consider this issue fixed with the patch #3.

Agreed! Views UI is too complicated, we should manage it on it's own.

Just a few minor code review points:

  1. +++ b/core/themes/seven/css/components/dropbutton.component.css
    @@ -25,20 +25,41 @@
    +  list-style-type:none;
    

    This requires a space before the value.

  2. +++ b/core/themes/seven/css/components/dropbutton.component.css
    @@ -25,20 +25,41 @@
    +.dropbutton li {
    ...
    +.dropbutton li:first-child {
    ...
    +.js .dropbutton li {
    

    Instead of using 'li' we use the '.dropbutton-action' class. It would be nice to keep it consistent.

Outi’s picture

Actually the '.dropbutton-action' class seems to be created by JS (when JS is deactivated the dropbutton 'li's don't have it). I don't know if this is normal; so should the '.dropbutton-action' class be there only when JS is activated (and in that case we must use 'li' here and not the class), or should the class be there in all cases ?

Outi’s picture

Ok, LewisNewman told me that we can keep the 'li' as the '.dropbutton-action' class is created by JavaScript, so this patch adds only a space before the value on the 'list-style-type: none;'.

Outi’s picture

Status: Needs work » Needs review
LoMo’s picture

Look great on my local Mac browsers, but I tried on Android and it needs work on smaller screens where the buttons are not in a row. See screenshot.

(According to the themer sitting to my left, we should: Remove the left margin from all of these buttons and add a margin-bottom.

LoMo’s picture

FileSize
113.25 KB
LoMo’s picture

Issue summary: View changes
Status: Needs review » Needs work

LoMo’s picture

Status: Needs work » Needs review
FileSize
41.22 KB
1.15 KB

This patch should get things okay. It only affects layout and adds just the margin-bottom and sets margin-left to zero.

Status: Needs review » Needs work

The last submitted patch, 14: dropdown_action_list-2349591-15.patch, failed testing.

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

This fixes the problem. I also tested to make sure that there were no regressions to Dropbuttons with JS.

droplet’s picture

Assigned: Unassigned » droplet
Status: Reviewed & tested by the community » Needs work

buttons should be 100% on mobiles. Patch is coming.

droplet’s picture

Assigned: droplet » Unassigned
Status: Needs work » Needs review
FileSize
1.3 KB
1.75 KB
11.92 KB
jerrylow’s picture

Looks good - screen shots for iPhone simulator, tested it for different iOS devices too.

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
amitgoyal’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +#dcdelhi
FileSize
1.76 KB

Re-roll of #19.

LewisNyman’s picture

Status: Needs review » Needs work
+++ b/core/misc/dropbutton/dropbutton.css
@@ -24,8 +24,10 @@
-.form-actions .dropbutton-wrapper {
-  float: left; /* LTR */
+@media screen and (min-width:600px) {
+  .form-actions .dropbutton-wrapper {
+    float: left;
+  }
 }
 [dir="rtl"] .form-actions .dropbutton-wrapper {
   float: right;

We lost the LTR comment on this line, also I guess this means the selector below it also needs to be moved in to the media query?

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
545 bytes

Fixes as per #23.

LewisNyman’s picture

Issue tags: +Needs screenshots

Great, thanks. We just need to take some screenshots of the JS dropbuttons in all the themes to show that it's not broken by these changes.

NGenius’s picture

Hello,

I am making screenshots.

LewisNyman’s picture

Issue summary: View changes
betovarg’s picture

Assigned: Unassigned » betovarg
betovarg’s picture

Assigned: betovarg » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
148.44 KB
146.97 KB
203.35 KB
143.73 KB
220.7 KB
141.98 KB
158.82 KB
160.35 KB
213.69 KB

I reviewed the behavior on chrome 40.0.2214.111 (64-bit) and firefox 35.0.1 on mac, on the needed screens. I tested on a local machine and simplytest.me (I verified on both, but screenshots are only for my local environment):

/admin/content
Seven

Classy

Bartik

/node/add/article
Seven

Classy

Bartik

/admin/structure/views/view/content
Seven

Classy

Bartik

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: dropdown_action_list-2349591-24.patch, failed testing.

aliyakhan’s picture

Looks all good in D8 8.0.x already. Attached couple of shots.

aliyakhan’s picture

Status: Needs work » Reviewed & tested by the community

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

CSS changes are not frozen in beta. Committed 71a987d and pushed to 8.0.x. Thanks!

  • alexpott committed 71a987d on 8.0.x
    Issue #2349591 by betovarg, Outi, LoMo, juho.lehmonen, droplet,...
idebr’s picture

This issue introduced a visual regression in the dropbutton for users with javascript enabled. I have opened #2430999: Visual regression in dropbutton to fix this.

Status: Fixed » Closed (fixed)

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