Improve menus UX: Remove "save menu item", swap "close" for "cancel", etc.

[koopersmith]

As per the conversation in dev chat, we found that saving individual menu items was impractical when used in conjunction with the "unsaved changes" AYS.

This patch is a first pass that tackles a few of the necessary changes and fixes a handful of bugs:

• Removes "save menu item".
• Swaps "close" for "cancel" and adds "cancel" JS functionality.
• Fixes delete menu item and menu update bugs.
• Several other minor bugs.

To do:

• Add "cancel" no-js functionality.
• Ensure unsaved menu items are not saved in any capacity (including as a draft).
• Copy unsaved menu data across no-js pages (so users can actually make changes to a menu).

[jane]

Agreed, per IRC discussion earlier today.

[nacin]

(In [14852]) First pass on better UX for menu item save. props koopersmith, see #13525.

[koopersmith]

The down arrow on menu items currently doesn't close the menu item details in no-js. Since we've removed the "close" link, we probably should patch that as well.

Also, if anyone is interested in patching any of the remaining tasks, feel free (though I will try and get to them over the next few days). If you have any questions about direction or implementation, ping me.

[duck_]

Just wondering if "Reset" might be more appropriate than "Cancel", it seems to me that "Cancel" implies closing the menu item dropdown as well as performing a reset action.

[jane]

"Reset" implies that all the field would be cleared, while "cancel" implies canceling the current action, which is the action under discussion.

[duck_]

Replying to jane:

"Reset" implies that all the field would be cleared, while "cancel" implies canceling the current action, which is the action under discussion.

Okay, fair enough.

[jane]

The starter text disappears as soon as you 'create menu' but when it blanks out we still need a prompt to add items from the left so users don't become disoriented.

Preferred solution: Make the empty menu container taller rather than empty after menu created but not yet filled, and add in center of it, "Select menu items (pages, categories, links) from the boxes at left to begin building your custom menu." Have it disappear on the first add.

[nacin]

(In [14917]) Don't hide early nav menu instructions until the menu is no longer empty. props koopersmith, see #13525

[jane]

The menu settings area seems to have some weird alignment, making it look less polished. The delete link and save button seem squashed up toward the top because of the empty space below created by the second line. The second line does not seem to left align with the first, and the checkbox seems bigger than the text height. Am on FF/Mac.

[duck_]

The meta boxes on the menus page always have the dropdown arrow showing, this is unlike all other meta boxes which only show this on hover. Also suggesting "on the left" instead of "at left". See patch.

[duck_]

Removal of a menu item should trigger AYS when you try to leave the page without saving: Remove an item; leave the page (no warning about unsaved content); go back and removed item is still there.

[duck_]

Replying to duck_:

Removal of a menu item should trigger AYS when you try to leave the page without saving: Remove an item; leave the page (no warning about unsaved content); go back and removed item is still there.

See 13525-remove_ays.diff.

[ocean90]

13525-AYS.patch: I think eventOnClickMenuItemDelete would be a better place for api.registerChange().

13525-fix-delete.patch: You cannot delete a menu item if it has post_status=draft, this patch should fix it.

[ocean90]

13525-no-js-cancel.patch: On cancel click the site will be reloaded and box will be open. To close the box you need to click on the arrow. Same behavior as in the JS version.

[nacin]

(In [14972]) Don't show expand/collapse menu bit for menu meta boxes except on hover. props duck_, see #13525.

[nacin]

Replying to duck_:

Also suggesting "on the left" instead of "at left". See patch.

Per IRC discussion, "at left" as it is shorter and still makes sense.

[nacin]

(In [14973]) Force menu AYS when a menu item is deleted. props ocean90, see #13525.

[nacin]

(In [14974]) Implement the Cancel menu item link for no JS. props ocean90, see #13525.

[nacin]

(In [14975]) gettext. see #13525.

[nacin]

(In [14977]) Clean up the menu name/actions section. see #13525.

[nacin]

I see nothing left on the to-do here, currently.

I would like to leave this ticket open for a final UX and bug sweep. It'd also be nice for koopersmith to confirm his to-do list is empty.

Random search issues: I'm noticing that the search doesn't fire if the last key was backspace. So if you make a typo or restrict your search too far and the ajax fires, it's a bit annoying to get it to re-fire. It also looks like it fires on a space, but ignores/trims it before executing the search.

[koopersmith]

Thanks for picking up the slack on this ticket, guys. Here are the issues I'm still seeing:

To Do:

• When a menu item is added, a draft of the item is created. If the menu is not saved, this item should be removed the next time the menu is edited. (Another possibility could be to remove draft items entirely.)

For No-JS:

• When a menu item is edited and then collapsed, its values are not copied into the now-hidden fields. This results in only being able to edit one item at once. These values are edited and unsaved, and should likely be POSTed as a result.

• When a menu item position is changed it saves the new positions to the DB. This results in menu item positions being updated live, instead of on save.

• The draft issue from above still pertains, but we also must ensure that draft menu items are not deleted while the user is mid-editing-session. One way of going about this could be to separate the generic 'edit' action from an 'editing' action, and remove draft items on the former.

• When cancel is clicked, all editable values (except position) should be restored to their original values. This appears to work now, but may break as the above are fixed.

I'll try and look into search issues if I have a minute, but I'm incredibly pressed for time.

[ocean90]

13525-minor-ie-6-7-tweaks.2.patch: Added border for IE 6 again. IE 6/7 fix for [14977]

[nacin]

I think there's enough on here to call the remaining tasks blockers. Allow me to add one more:

The JS is not RTL-safe. Switch $wp_locale->text_direction to RTL and you'll see some problems with both positioning menu item hierarchy and saving it. Going to ping koopersmith but he is traveling over the next two days.

[ocean90]

13525-dont-show-drafts.patch: first todo fixed.

[nacin]

(In [14993]) Some minor IE6/7 tweaks for menus. props ocean90, see #13525.

[nacin]

Replying to ocean90:

13525-dont-show-drafts.patch: first todo fixed.

We can't just not show them, we need to clean them up as well I'd think, no?

[automattor]

(In [14994]) RTL string fix. see #13525.

[yoavf]

Update to nav menus RTL

[nacin]

(In [14997]) RTL menu updates. props yoavf, see #13525.

[nacin]

RTL - [14997]

[nacin]

D'oh, still need to work on RTL in the JS.

[filosofo]

Breaking the menu item draft issue into #13579

[automattor]

(In [15007]) Remove some dead code. props ocean90, see #13525.

[nacin]

(In [15010]) Menu touchups. see #13525.

[ocean90]

point 2 of todo

[filosofo]

nav-menu-rtl-js.13525.2.diff provides rtl fixes just for the drag and dropping; I've taken out my misguided attempts to deal with the tab scrolling in the previous patch.

[koopersmith]

Looks good to me. Taking the absolute value might cause the menu item to indent if the user drags the item in the opposite direction though. If that's the case we should just reverse the args or multiply by -1 if rtl.

[filosofo]

OK, nav-menu-rtl-js.13525.3.diff reverses sign for rtl on depth instead of absolute value.

[ryan]

(In [15039]) Nav menu RTL js fixes. Props filosofo. see #13525

[nacin]

Todo:

• tab scrolling in RTL, followed by final RTL sweep. koopersmith?
• whatever 13525-no-js-if-close-item-save-data.2.patch is trying to achieve. ocean90?
• final menus UX sweep in both JS and non-JS, particularly strings. jane, koopersmith, filosofo, etc.

[filosofo]

If koopersmith is not available to do tab scrolling, I'll do it. Also, I do know what the rtl problems with it are if anyone wants to chat about it.

[duck_]

13525-delete-double-ays.diff: Create a new menu, add some items (don't save), decide you don't want a menu and click Delete Menu... you now face two AYS notifications, first the deletion one and then there is unsaved stuff note.

[nacin]

From ocean90, some translation issues: ​http://grab.by/4D8v

[filosofo]

stop-unloads-in-appropriate-places.diff takes duck_'s patch and puts the onuload event canceler with an already existing event listener, so no double event listening.

[nacin]

ocean90's padding adjustments: ​http://grab.by/4Dcn. I imagine moving from "View All" to "All" will should keep most translators from overdoing it.

Technically we should have context on these strings (the context being the post type or taxonomy), but we don't use that for page filters so I think we can avoid it in 3.0.

[nacin]

(In [15044]) Give translators a little more room with nav menu meta box tabs. see #13525.

[nacin]

(In [15045]) Make nav menu meta box tab ordering consistent across post types and taxonomies. see #13525.

[nacin]

(In [15046]) Don't show double-AYS for deleting a menu. props filosofo, duck_. see #13525.

[nacin]

Everything here should be committed. Todo outlined in comment 41 still stands.

[koopersmith]

I'm back on solid ground and will take a shot at tabs today/tomorrow (there are a few things I want to tweak there anyway). I'll try and be available for a UX sweep as well.

[nacin]

Suggestion, in non-JS, after the custom link box, the pages and categories meta boxes should come next (in that order), followed by all other post type and taxonomy boxes.

[ocean90]

nacin:

whatever 13525-no-js-if-close-item-save-data.2.patch is trying to achieve. ocean90?

See comment:23 point 1 of non-js.

[koopersmith]

13525.tabs.diff adds rtl support for tabs, shows/hides the arrows when needed, improves scrolling, and provides a handful of minor fixes/improvements.

[nacin]

(In [15080]) Nav menu improvements. props koopersmith. RTL support for tabs, better tab handling, other improvements. see #13525.

[nacin]

Here's what I see left on the todo:

• whatever 13525-no-js-if-close-item-save-data.2.patch is trying to achieve. I believe filosofo had an issue with the current patch based on how and when we would be saving items being edited, IIRC.
• Final RTL sweep.
• final menus UX sweep in both JS and non-JS, particularly strings.

Can we finish this today?

[ocean90]

We have stil a layout bug in IE 7, see ​http://grab.by/4ImQ. It comes with [14997].

[ocean90]

Hide the close link for non-js

[nacin]

(In [15122]) Hide the 'Close' link in nav menu no-JS. The Cancel link should suitably handle the modified workflow. props ocean90, see #13525.

[nacin]

Needs patch to fix IE7 in RTL.

[ocean90]

Working on IE 7 patch now.

[ocean90]

Forget the comment. Patch is there.

[ocean90]

Do it only for the nav menus page.

[nacin]

(In [15214]) Horizontal scrolling/resizing fixes for nav menu UI. Also some RTL fixes. props koopersmith, see #13525

[koopersmith]

This patch should do it for rtl in IE7, and keeps IE6 in line as well.

[ryan]

[15218]