WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

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

Download all attachments as: .zip

Change History (94)

koopersmith4 years ago

koopersmith4 years ago

comment:1 jane4 years ago

Agreed, per IRC discussion earlier today.

comment:2 nacin4 years ago

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

comment:3 koopersmith4 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.

comment:4 duck_4 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.

comment:5 follow-up: jane4 years ago

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

comment:6 in reply to: ↑ 5 duck_4 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.

comment:7 jane4 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.

comment:8 nacin4 years ago

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

comment:9 nacin4 years ago

  • Component changed from General to Menus

comment:10 jane4 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.

comment:11 follow-up: duck_4 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.

comment:12 follow-up: duck_4 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.

duck_4 years ago

comment:13 in reply to: ↑ 12 duck_4 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.

ocean904 years ago

ocean904 years ago

comment:14 ocean904 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.

ocean904 years ago

comment:15 ocean904 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.

comment:16 nacin4 years ago

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

comment:17 in reply to: ↑ 11 nacin4 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.

comment:18 nacin4 years ago

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

comment:19 nacin4 years ago

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

comment:20 nacin4 years ago

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

comment:21 nacin4 years ago

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

comment:22 nacin4 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.

comment:23 koopersmith4 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.

comment:24 ocean904 years ago

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

comment:25 nacin4 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.

comment:26 follow-up: ocean904 years ago

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

comment:27 nacin4 years ago

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

comment:28 in reply to: ↑ 26 nacin4 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?

comment:29 automattor4 years ago

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

comment:30 nacin4 years ago

  • Keywords needs-rtl added

yoavf4 years ago

Update to nav menus RTL

comment:31 nacin4 years ago

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

comment:32 nacin4 years ago

  • Keywords needs-rtl removed

RTL - [14997]

comment:33 nacin4 years ago

  • Keywords needs-rtl added

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

comment:34 filosofo4 years ago

Breaking the menu item draft issue into #13579

comment:35 automattor4 years ago

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

comment:36 nacin4 years ago

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

ocean904 years ago

point 2 of todo

comment:37 filosofo4 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.

comment:38 koopersmith4 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.

comment:39 filosofo4 years ago

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

comment:40 ryan4 years ago

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

comment:41 nacin4 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.

comment:42 nacin4 years ago

  • Keywords ux-feedback added

comment:43 filosofo4 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.

comment:44 duck_4 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.

comment:45 nacin4 years ago

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

comment:46 filosofo4 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.

comment:47 nacin4 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.

comment:48 nacin4 years ago

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

comment:49 nacin4 years ago

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

comment:50 nacin4 years ago

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

comment:51 nacin4 years ago

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

comment:52 koopersmith4 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.

comment:53 nacin4 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.

comment:54 ocean904 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.

koopersmith4 years ago

comment:55 koopersmith4 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.

comment:56 nacin4 years ago

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

comment:57 nacin4 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?

comment:58 nacin4 years ago

  • Severity changed from blocker to major

comment:59 ocean904 years ago

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

ocean904 years ago

Hide the close link for non-js

comment:60 nacin4 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.

comment:61 nacin4 years ago

  • Keywords needs-patch added; has-patch removed

Needs patch to fix IE7 in RTL.

comment:62 ocean904 years ago

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

Working on IE 7 patch now.

ocean904 years ago

comment:63 ocean904 years ago

  • Keywords has-patch added; needs-patch removed

Forget the comment. Patch is there.

ocean904 years ago

Do it only for the nav menus page.

comment:64 nacin4 years ago

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

koopersmith4 years ago

comment:65 koopersmith4 years ago

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

comment:66 ryan4 years ago

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

comment:67 ocean904 years ago

  • Keywords rtl-feedback removed

nacin4 years ago

Note: See TracTickets for help on using tickets.