Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford’s picture

Patch applies nicely. Checked it in FF, Safari, & Chrome in the mac & it all worked well. I tested in FF without javascript and the link didn't show up (so all was good).

It didn't work for me in Opera 9.62 though. The 'Show advanced settings' link showed up (and the ENABLED & SHOW AS EXPANDED were hidden by default), but nothing happened when I clicked on the link.

I do like the idea behind this patch. I think so often people are overwhelmed by the options in Drupal. I do think that someone should be able to work with Drupal core using Opera though.

jim0203’s picture

Doesn't seem to work in Opera 10.0 on the Mac - clicking the link does nothing. Works fine in FF 3.5 and Safari 4 on Mac. Works fine in all three browsers with JavaScript turned off.

+1 for this patch being a great idea and completely in-keeping with d7ux.

If someone can tell me how I can access my OS X XAMPP webserver from the Windows 7 and Ubuntu machines I've got virtualised via Virtualbox then I can test this for browsers in those OSs as well.

clemens.tolboom’s picture

Works on FF 3.0.13 on Ubuntu 9.04

* Added some linewrapping
* Rewrote if into inline if (:) ... same code same layout :)

ksenzee’s picture

I'll try to track down the Opera issue. We definitely need this patch if we use the menu system for shortcuts.

ksenzee’s picture

+++ modules/menu/menu.admin.js	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,41 @@
+      self = $(this)

Found it. We need a var declaration here for Opera: var self = $(this)

Otherwise this looks great.

I'm on crack. Are you, too?

ksenzee’s picture

Oops, forgot to mention that we should go back to if-else. The ternary operator is notoriously difficult to follow, and jQuery is hard enough without adding more complexity. :) Other than *that* this looks great.

dmitrig01’s picture

Ok, fixed. Please review in opera.

dmitrig01’s picture

Ternaries removed.

dmitrig01’s picture

With JS ;-)

ksenzee’s picture

Status: Needs review » Reviewed & tested by the community

Lovely. Works in Opera now, and is a nice UI improvement. RTBC.

mgifford’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
63.11 KB

I can confirm that this patch still applies nicely and that this time it's working great with Opera on the mac too!

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

I was editing this at the same time as ksenzee, setting it back to RTBC.

David_Rothstein’s picture

Yeah, this works well.

In #511286: D7UX: Implement customizable shortcuts we had this same feature working without JavaScript, which is in some ways better (more accessible, less stuff loaded on the page), but in some ways worse (you can't switch back and forth between modes without saving your changes). I think the fact that the switch for this is now right above the table - where it indeed is much better UX than what I had before - argues a bit in favor of the JavaScript solution, since it means people will be tempted to click on it before their changes are saved.

Also, the patch in #511286: D7UX: Implement customizable shortcuts had a feature where the "simplified" interface did not give you access to the complete menu tree (N levels deep) but rather only the top level. This is especially useful for the shortcuts, but also for other menus (at least in my opinion) - a lot of the time, you really don't want/need to see the full menu tree on the page, and it just gets in the way. Maybe as a followup patch we can look at that too?

Bojhan’s picture

Status: Reviewed & tested by the community » Needs review

Will revisit this after code freeze, not sure if we should go down this path - i.e is this a reusable pattren?

ksenzee’s picture

Hi Bojhan. From reading just this issue, it's totally unclear that this patch is currently being used in #511286: D7UX: Implement customizable shortcuts as the interface for editing shortcut bar settings. I wanted to make sure you were aware of that, in case you're not a mindreader. :) I'm not married to this UI, but we need either this or an alternative for editing the shortcut bar. Otherwise people will have to wade into the entire complicated menu system UI just to change things around on their shortcut bar.

Dries’s picture

I think this patch is an artifact of a (potentially) poor design decision in #511286: D7UX: Implement customizable shortcuts. I'm not keen to introduce this pattern.

David_Rothstein’s picture

FileSize
51.79 KB

I don't think this has any dependence on #511286: D7UX: Implement customizable shortcuts. It seems to me like a good idea in its own right. 95% of the time when I edit a menu, all I need to do is edit a link or rearrange items. The advanced settings just get in the way.

For some menus, settings like "show as expanded" make even less sense. The shortcuts menu is one example, but not the only one. For example, the top level of the toolbar has the same issue; it will never show its items as "expanded" regardless of what option you choose in the menu UI. So Drupal is pretty confusing in that regard.

Also, the "enabled" checkbox is a particularly good candidate for hiding by default, since it's rarely needed, and disabled menu items are already labeled as such in the UI, so it's not even providing extra information either... (See the attached screenshot.)

I agree it's worth thinking a bit more about whether this is a reusable pattern. One way to improve it might be to print the text for other settings in a similar way as "disabled" -- e.g., if I have a menu item that is expanded, it could say "expanded by default" next to it, and that way I still get the information when I need it, but I don't need to have a giant column of checkboxes on my screen that will be unchecked 99% of the time. In that sense, it might have a bit in common with the pattern already in use for Vertical Tabs (print the data, but hide the widget for editing it)?

David_Rothstein’s picture

Title: Toggle advanced settings on menu settings » Remove advanced settings from the menu overview page
Status: Needs review » Needs work

It occurs to me now that maybe we are missing an obvious solution to this problem.

What is the rationale for these two columns of checkboxes existing on this screen at all, for any menu, ever? Can't we just remove them?

If you need to change these settings, you can get to them by clicking "edit" next to any of the menu links - they show up on that screen as well. So I don't think I even see the reason for showing them prominently as a row of checkboxes on the overview page. The only reason to do that would be if they were a commonly used setting (where people would often need to click-click-click change a bunch of them at once), but I'm really having trouble thinking of any use case where you would do that.

So what if instead of hiding them via JavaScript, we just removed them from that page completely? I think I even have some code at the other issue that mostly does that already...

JacobSingh’s picture

Status: Needs work » Needs review
FileSize
3.36 KB

I'm totally in agreement here. These are two barely used settings which are given huge precedence without any use case to support it. If we want to make menu screen easier to use, this wouldn't be a bad start.

Even if it came out of the other issue, it does seem like worth doing rather than form altering it in somewhere else. The fact you can get at them with the Edit links makes it a sealed deal for me.

I also don't see a need for an "advanced" tab - that seems like a bad way to approach the problem. As David points out, you only need to edit in a grid if you are planning to operate on multiple entries at once, and I see no reason why you would do that with either of those text boxes.

Let's KISS.

Patch is attached. Here is the screen - click to view.

Administration shortcuts | My Site

Best,
Jacob

Gábor Hojtsy’s picture

I'm a little concerned with people deleting menu items when they'd actually want to disable it. Setting up a menu item or tree again can be painful.

Also, I'm seeing you lowered the colspan on the operations column, but the content of the operations column did not change. Was that a bogus value already?

On the width of the operations column, we had a long discussion on that with Bojhan before. It needs to be evaluated on a per-screen basis.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

Status: Needs work » Needs review
FileSize
3.16 KB

Discussed with Gabor.

Agreed about the disabling / deleting. One option is to make disabling the operation, and remove deleting. This makes a lot more sense to me. For now, this leaves the enabled column there, but gives it a wary eye.

Also adds some styling (possibly poor) to make the table a little easier to look at.

Only local images are allowed.

Best.
J

David_Rothstein’s picture

FileSize
30.3 KB

Attached is a screenshot showing Jacob's latest patch.

I agree that moving "disable" to the operations column makes the most sense. I had forgotten before that there is one major case where that is actually a pretty common operation: menu items generated by modules, which (as can be seen from the attached screenshot) cannot be deleted, only disabled.

I don't think there's any reason to remove the "delete" operation from this column, though - it's there now, and the world hasn't ended yet :) There is a confirmation form when you click on it to help avoid people making mistakes (and arguably, the "disable" link doesn't need a confirmation form, just a token, since it is completely and easily reversible).

So I think "edit + disable + delete" (or "edit + enable + delete", of course) makes sense. Disabling and enabling menu items is definitely an operation, so this would be consistent with the rest of Drupal where the operations are all listed as links in the right column.

JacobSingh’s picture

I actually take back my comment about moving disabled to the ops column. There is not a huge use case for disabling a bunch of menu items at once, but I've certainly done it before. The stock Drupal Navigation menu has a bunch of stuff many admins don't want to show, so I can see why they'd want a quick place to do it. Plus, it's not really a big deal UI wise or code wise.

Anyway - we just need a call now on if this goes in.

David_Rothstein’s picture

If we move "disabled" to the operations column but don't put a confirmation form behind it, it should be almost as fast as a row of checkboxes...

JacobSingh’s picture

Yeah, sounds fine to me, even better if we have a JS enhancement to AJAX it, don't want to see it holding up the patch though.

catch’s picture

Category: feature » task
Status: Needs review » Needs work

Let's not introduce a new pattern for AJAX operations links which don't go to forms. The idea in general could grow on me - clicking a single AJAX link is a lot quicker than clicking a single checkbox then a button, but that needs it's own issue.

Just removing the expanded checkbox would already be a massive improvement. I'd suggest we do that, and open a new issue for nifty operations links / disabled.

JacobSingh’s picture

Status: Needs work » Needs review

So I think this needs review then, right?

Currently, doesn't do the ajaxy link thing, just removed the checkbox and styles the table a bit.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

Status: Needs work » Needs review
FileSize
3.29 KB

Okay, now that I know what tabs he meant!

catch’s picture

Title: Remove advanced settings from the menu overview page » Remove 'expanded' checkbox from menu item overview
Status: Needs review » Reviewed & tested by the community

Screenshot in #23 is still accurate. RTBC from me.

Dries’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Usability

Looks simpler to me, and I agree that this covers the 80% use case perfectly fine. And with the extra CSS, it looks visually better too. Committed to CVS HEAD. Thanks all.

Status: Fixed » Closed (fixed)
Issue tags: -Usability, -D7UX

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

Status: Closed (fixed) » Needs review
Issue tags: +Usability, +D7UX

2biazdk requested that failed test be re-tested.

David_Rothstein’s picture

Status: Needs review » Closed (fixed)
himerus’s picture

For those that find this issue because you DO wonder where the "expanded" checkboxes went on the menu_overview_form, I have added a module: http://drupal.org/project/menu_expanded which adds them back in for those who DO like them on the interface, and need to set a lot of menu items to expanded quickly, and on the same screen as we were used to prior to D7.

I agree with this issue, that it IS a case where that is "rarely" used, or only used once, but i think that having to click "edit" then set to expanded, then save, then repeat for each and every menu item is a pain.

http://drupal.org/project/menu_expanded

temmes’s picture

Status: Closed (fixed) » Needs review

30: 564886-simple_menu_admin.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 30: 564886-simple_menu_admin.patch, failed testing.

yoroy’s picture

Issue summary: View changes
Status: Needs work » Fixed

Lets leave this fixed. Any specific reason you reopened this?

Status: Fixed » Closed (fixed)

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