WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#13525 closed defect (bug) (fixed)

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

Reported by: koopersmith Owned by: ocean90
Milestone: 3.0 Priority: high
Severity: major Version: 3.0
Component: Menus Keywords: has-patch ux-feedback
Focuses: Cc:

Description

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).

Attachments (27)

13525.diff (14.4 KB) - added by koopersmith 7 years ago.
13525.2.diff (14.4 KB) - added by koopersmith 7 years ago.
13525.instructions.diff (2.5 KB) - added by koopersmith 7 years ago.
13525-handlediv_and_help.diff (1.2 KB) - added by duck_ 7 years ago.
13525-remove_ays.diff (392 bytes) - added by duck_ 7 years ago.
13525-AYS.patch (451 bytes) - added by ocean90 7 years ago.
13525-fix-delete.patch (703 bytes) - added by ocean90 7 years ago.
13525-no-js-cancel.patch (2.0 KB) - added by ocean90 7 years ago.
13525-minor-ie-6-7-tweaks.patch (673 bytes) - added by ocean90 7 years ago.
13525-minor-ie-6-7-tweaks.2.patch (1.6 KB) - added by ocean90 7 years ago.
13525-dont-show-drafts.patch (762 bytes) - added by ocean90 7 years ago.
3.0-nav-menus-rtl.css (4.9 KB) - added by yoavf 7 years ago.
Update to nav menus RTL
13525-no-js-if-close-item-save-data.2.patch (1.8 KB) - added by ocean90 7 years ago.
point 2 of todo
nav-menu-rtl-js.13525.diff (3.9 KB) - added by filosofo 7 years ago.
nav-menu-rtl-js.13525.2.diff (1.9 KB) - added by filosofo 7 years ago.
nav-menu-rtl-js.13525.3.diff (1.9 KB) - added by filosofo 7 years ago.
13525-delete-double-ays.diff (434 bytes) - added by duck_ 7 years ago.
stop-unloads-in-appropriate-places.diff (1020 bytes) - added by filosofo 7 years ago.
a_little_bit_space_for_translation.patch (1.1 KB) - added by ocean90 7 years ago.
13525.tabs.diff (5.4 KB) - added by koopersmith 7 years ago.
hide-close.patch (406 bytes) - added by ocean90 7 years ago.
Hide the close link for non-js
13525-ie-fix.patch (822 bytes) - added by ocean90 7 years ago.
13525-ie-fix.2.patch (1.6 KB) - added by ocean90 7 years ago.
Do it only for the nav menus page.
13525.horizontal.scrolling.and.friends.diff (8.8 KB) - added by koopersmith 7 years ago.
13525.ie7.rtl.patch (2.0 KB) - added by koopersmith 7 years ago.
13525.ie7.rtl.2.patch (2.1 KB) - added by koopersmith 7 years ago.
13822.diff (9.4 KB) - added by nacin 6 years ago.

Download all attachments as: .zip

Change History (94)

@koopersmith
7 years ago

@koopersmith
7 years ago

#1 @jane
7 years ago

Agreed, per IRC discussion earlier today.

#2 @nacin
7 years ago

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

#3 @koopersmith
7 years ago

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.

#4 @duck_
7 years ago

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.

#5 follow-up: @jane
7 years ago

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

#6 in reply to: ↑ 5 @duck_
7 years ago

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.

#7 @jane
7 years ago

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.

#8 @nacin
7 years ago

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

#9 @nacin
7 years ago

  • Component changed from General to Menus

#10 @jane
7 years ago

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.

#11 follow-up: @duck_
7 years ago

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.

#12 follow-up: @duck_
7 years ago

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.

#13 in reply to: ↑ 12 @duck_
7 years ago

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
7 years ago

#14 @ocean90
7 years ago

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.

#15 @ocean90
7 years ago

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.

#16 @nacin
7 years ago

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

#17 in reply to: ↑ 11 @nacin
7 years ago

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.

#18 @nacin
7 years ago

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

#19 @nacin
7 years ago

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

#20 @nacin
7 years ago

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

#21 @nacin
7 years ago

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

#22 @nacin
7 years ago

  • Keywords has-patch removed

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.

#23 @koopersmith
7 years ago

  • Keywords needs-patch added

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.

#24 @ocean90
7 years ago

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

#25 @nacin
7 years ago

  • Priority changed from normal to high
  • Severity changed from normal to blocker

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.

#26 follow-up: @ocean90
7 years ago

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

#27 @nacin
7 years ago

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

#28 in reply to: ↑ 26 @nacin
7 years ago

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?

#29 @automattor
7 years ago

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

#30 @nacin
7 years ago

  • Keywords needs-rtl added

@yoavf
7 years ago

Update to nav menus RTL

#31 @nacin
7 years ago

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

#32 @nacin
7 years ago

  • Keywords needs-rtl removed

RTL - [14997]

#33 @nacin
7 years ago

  • Keywords needs-rtl added

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

#34 @filosofo
7 years ago

Breaking the menu item draft issue into #13579

#35 @automattor
7 years ago

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

#36 @nacin
7 years ago

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

#37 @filosofo
7 years ago

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.

#38 @koopersmith
7 years ago

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.

#39 @filosofo
7 years ago

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

#40 @ryan
7 years ago

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

#41 @nacin
7 years ago

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.

#42 @nacin
7 years ago

  • Keywords ux-feedback added

#43 @filosofo
7 years ago

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.

#44 @duck_
7 years ago

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.

#45 @nacin
7 years ago

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

#46 @filosofo
7 years ago

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.

#47 @nacin
7 years ago

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.

#48 @nacin
7 years ago

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

#49 @nacin
7 years ago

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

#50 @nacin
7 years ago

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

#51 @nacin
7 years ago

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

#52 @koopersmith
7 years ago

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.

#53 @nacin
7 years ago

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.

#54 @ocean90
7 years ago

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.

#55 @koopersmith
7 years ago

  • Keywords has-patch added; needs-patch removed

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

#56 @nacin
7 years ago

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

#57 @nacin
7 years ago

  • Keywords rtl-feedback added; needs-rtl removed

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?

#58 @nacin
7 years ago

  • Severity changed from blocker to major

#59 @ocean90
7 years ago

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

@ocean90
7 years ago

Hide the close link for non-js

#60 @nacin
7 years ago

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

#61 @nacin
7 years ago

  • Keywords needs-patch added; has-patch removed

Needs patch to fix IE7 in RTL.

#62 @ocean90
7 years ago

  • Owner changed from koopersmith to ocean90
  • Status changed from new to accepted

Working on IE 7 patch now.

#63 @ocean90
7 years ago

  • Keywords has-patch added; needs-patch removed

Forget the comment. Patch is there.

@ocean90
7 years ago

Do it only for the nav menus page.

#64 @nacin
7 years ago

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

#65 @koopersmith
7 years ago

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

#66 @ryan
7 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

#67 @ocean90
7 years ago

  • Keywords rtl-feedback removed

@nacin
6 years ago

Note: See TracTickets for help on using tickets.