Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#23450 closed task (blessed) (fixed)

Refactor menu item meta boxes as accordion

Reported by: lessbloat's profile lessbloat Owned by: markjaquith's profile markjaquith
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Menus Keywords: 3.6-menus has-patch needs-codex
Focuses: Cc:

Description

Once #23449 is complete, we are going to refactor the menu items meta boxes on nav-menus.php to reflect the same design as the accordion in the customizer. We've already prototyped & tested this design on users, and it seems to provide a better UI.

Attachments (17)

23450.diff (18.4 KB) - added by lessbloat 11 years ago.
23450.1.diff (48.7 KB) - added by lessbloat 11 years ago.
23450.1.refactored.diff (30.4 KB) - added by lessbloat 11 years ago.
Removed code from 23449.2.diff (which was committed yesterday)
23450.2.diff​ (30.6 KB) - added by lessbloat 11 years ago.
23450.2.diff (30.6 KB) - added by DrewAPicture 11 years ago.
refresh
23450.3.diff (33.4 KB) - added by lessbloat 11 years ago.
23450.4.diff (34.9 KB) - added by jkudish 11 years ago.
restores screen options and sweep for missing escaping + code standards (spacing)
23450.5.diff (35.6 KB) - added by lessbloat 11 years ago.
23450.6.diff (36.7 KB) - added by lessbloat 11 years ago.
23450.7.diff (36.3 KB) - added by DrewAPicture 11 years ago.
Screen options attribute toggles
23450.8.diff (36.3 KB) - added by DrewAPicture 11 years ago.
post_types ordering
23450.9.diff (14.2 KB) - added by DrewAPicture 11 years ago.
New approach w/ do_menu_accordion_sections()
23450.10.diff (14.6 KB) - added by lessbloat 11 years ago.
23450.11.diff (19.4 KB) - added by lessbloat 11 years ago.
23450.12.diff (17.0 KB) - added by DrewAPicture 11 years ago.
Removes add|remove wrappers, renames do_accordion_sections()
23450.13.diff (17.1 KB) - added by DrewAPicture 11 years ago.
Minor cleanup, move outer accordion markup into do_accordion_sections() as with do_meta_boxes()
23450.14.diff (7.0 KB) - added by DrewAPicture 11 years ago.
DRY-er approach

Download all attachments as: .zip

Change History (67)

#1 @DrewAPicture
11 years ago

  • Cc xoodrew@… added

#2 @lessbloat
11 years ago

Sorry, 23450.diff​ was uploaded by mistake... It was supposed to be uploaded to #23449.

Last edited 11 years ago by lessbloat (previous) (diff)

@lessbloat
11 years ago

#3 @sabreuse
11 years ago

  • Cc sabreuse added

#4 @lessbloat
11 years ago

  • Keywords needs-testing added; needs-patch removed

#5 @DrewAPicture
11 years ago

23450.diff shows no noticeable differences, so that's good.

One question I have is whether we're:

A) Breaking this out so that it can be reused in core only

B) Breaking this out for core + public use?

Whichever way we go, we should probably at the very least lay out basic usage in a docblock somewhere (maybe accordion.js)

@lessbloat
11 years ago

#6 @lessbloat
11 years ago

23450.1.diff​ is a really, really rough first stab at this.

A couple notes:

  • I'm honestly not sure if I'm helping or hindering the progress of this task by posting this patch (i.e. I think I'm in over my head ;-))
  • This patch includes 23449.2.diff​ as a base. It's not super clean, but what can you do? 23450.1.diff​ relies on 23449.2.diff, and 23449.2.diff has yet to be committed...
  • With 23450.1.diff​ applied you'll see:

http://f.cl.ly/items/3W1S1I3C3r100V2E2z05/menu-items-accordion.jpg

Where I need help

  • Screen options disappeared. I don't know how to get them back
  • Hidden sections (i.e. pages, tags, formats) are not being hidden by default
  • Refactoring anything that needs it on the back-end

After that is fixed

  • I'll then swing back around and polish up the styles (it's all still a bit rough looking ATM)
  • I'll fix the tabs (i.e Most Recent, View All, Search)
  • I'll add JS to auto show the first menu item option (currently they all start closed on page load).

And then we should be good to go...

Last edited 11 years ago by lessbloat (previous) (diff)

@lessbloat
11 years ago

Removed code from 23449.2.diff (which was committed yesterday)

#7 follow-up: @jkudish
11 years ago

@lessbloat unfortunately 23450.1.refactored.diff no longer applies cleanly for me against WordPress trunk. Can you please update the patch so it can apply cleanly so I can take a look and help you with the few points you need help with?

#8 @DrewAPicture
11 years ago

  • Keywords has-patch needs-refresh added

#9 in reply to: ↑ 7 @lessbloat
11 years ago

Replying to jkudish:

@lessbloat unfortunately 23450.1.refactored.diff no longer applies cleanly for me against WordPress trunk. Can you please update the patch so it can apply cleanly so I can take a look and help you with the few points you need help with?

Sorry about that. See 23450.2.diff.

#10 @jkudish
11 years ago

  • Cc jkudish added

@DrewAPicture
11 years ago

refresh

#11 @DrewAPicture
11 years ago

  • Keywords needs-refresh removed
  • Type changed from defect (bug) to enhancement
  • Version set to trunk

#12 @DrewAPicture
11 years ago

Testing 23450.2.diff:

@lessbloat
11 years ago

#13 @lessbloat
11 years ago

23450.3.diff​ has a few tweaks:

  • Made side border darker once expanded
  • Rounded top and bottom corners
  • Removed excess margin-top from .accordion-container
  • Fixed up tabs JS (i.e. most recent, all, search)

@jkudish
11 years ago

restores screen options and sweep for missing escaping + code standards (spacing)

@lessbloat
11 years ago

#14 @lessbloat
11 years ago

2 small tweaks in 23450.5.diff:

  • Moves Pages to the top
  • Auto opens the top menu items accordion option

@lessbloat
11 years ago

#15 @lessbloat
11 years ago

23450.6.diff​ adds a default container when there are zero posts.

#16 @chipbennett
11 years ago

Can I throw in a vote for returning the “Theme Locations” meta box? IMHO it is far more intuitive to associate Theme Locations with menus, than to associate menus with Theme Locations. It is also far more useful to see, at a glance, what Theme Locations have assigned menus, than to see what Theme Locations a given menu is assigned to.

To test:

Step X: Assign a custom menu to every Theme Location provided by the current Theme

I think this step – critical to the setup of any given Theme – has been made more difficult with the current UI changes.

Moving over some of the discussion from Make/UI:

The positioning of the theme locations meta box was confusing. It was aligned in the left column at the top…

Agreed. But why not put the Theme Locations metabox in the main column, right above the Menu-Edit metabox?

The new approach reduces this to two steps.

But the reduction-in-steps isn’t associated directly with the change. After creating a new menu, the user must still assign the menu to a metabox – only using checkboxes now instead of the dropdown.

Also, this workflow appears to assume: a) a New Theme, with b) no previously created custom nav menus. Have you done UI testing for a user who already has created custom nav menus, then switches Themes?

Also, you can see at a glance which theme locations are assigned to which menus in the menu select drop down

“At a glance” requires clicking the select, in order to see all menus/Theme locations. That’s an extra step from the old UI, when all Theme Locations were visible without user interaction. Also, I’ve not tested: how does the current UI scale when the same menu is applied to multiple Theme Locations?

You also cannot tell from that select whether all Theme Locations have menus assigned.

The only way to know if there are any Theme Locations without menus assigned is to make a mental note of the assigned locations in the dropdown at the top of the page, then compare that mental list with the list of checkboxes at the bottom of the Edit-Menu metabox.

IMHO, that is a pretty significant UI loss inherent in this change.

#17 follow-up: @chipbennett
11 years ago

Mocking up wireframes isn't my forte; but I can try to describe an alternative that might make a decent compromise:

  • Upper-left: "Select menu to edit" dropdown (same column width as the menu item selection metaboxes beneath)
  • Upper-right: Theme Locations UI
  • Bottom-left: existing menu item selection meta boxes
  • Bottom-right: existing menu-edit metabox

This layout would make use of the currently unused space at the top center/right of the screen (that's a lot of wasted, prime real estate, anyway).

#18 in reply to: ↑ 17 @lessbloat
11 years ago

Replying to chipbennett:

A couple of thoughts:

A) There is no reason to kick up the exact same conversation in two spots (both here and on the make/UI P2). I assure you that your feedback has been heard. In the future, can we please try to keep it to a single thread?

B) This is probably not the best ticket to bring up feedback about "theme locations". In fact "theme locations" wasn't mentioned at all, until your comment. If you'd like to discuss this in a ticket, please create a new ticket.

C) We've literally been discussing the revamp of menus for the 3.6 release for what, the past 2 months (with office hours twice a week)? And you pick now (6 days before hard freeze on feature development) to suggest that we start all over? :-)

D) I think that the underlying difference is you are looking at it purely from a "power users" standpoint, and I'm looking at it from a "majority of our users" standpoint. Unfortunately, you can't have it both ways. The old menus screen was miserable for the majority of users, plain and simple. The steps we took in this cycle were to eliminate complexity for the majority of the users.

E) Ultimately the decision is not mine (I'm not a core committer, nor a lead), but I'm happy with the progress that we've made in simplifying the menus screen this cycle.

Last edited 11 years ago by lessbloat (previous) (diff)

#19 follow-up: @chipbennett
11 years ago

As-noted in the blog post, I wanted to move discussion to Trac, as it seems more appropriate.

If you'd like to discuss this in a ticket, please create a new ticket.

I'd prefer to comment on it in the ticket in which the change was introduced. I apologize if I picked the wrong ticket. I assumed it was this one, as a quick scan of "Menus" component trac tickets didn't reveal one that looked like a more likely candidate. I'll be happy to have the conversation in the correct ticket.

We've literally been discussing the revamp of menus for the 3.6 release for what, the ​past 2 months (with office hours twice a week)? And you pick now (6 days before hard freeze on feature development) to suggest that we start all over? :-)

I don't spend much time in the Appearance -> Menus screen, so while I've been running Alpha all along, I really didn't notice this change, or realize its impact, until the Theme Review Queue push last week.

I think that the underlying difference is you are looking at it purely from a "power users" standpoint, and I'm looking at it from a "majority of our users" standpoint. Unfortunately, you can't have it both ways. The old menus screen was miserable for the majority of users, plain and simple. The steps we took in this cycle were to eliminate complexity for the majority of the users.

A majority of users switch Themes frequently. Thus, a majority of users will have already created custom nav menus when switching Themes. Have the UI tests accounted for this use case? Because it appears that every single one of the user tests, as documented on the Make/UI site, includes:

Step 3: Add a menu

The UI is considerably more intuitive under the assumption that the user will add a new menu every time the UI is used. It is considerably less intuitive when working with already created menus.

If that use case hasn't been tested, then yes: I think it is entirely valid to question whether it is ready for feature freeze.

#20 @cais
11 years ago

  • Cc edward.caissie@… added

#21 in reply to: ↑ 19 ; follow-up: @lessbloat
11 years ago

Replying to chipbennett:

To clarify, more or less, what I'm hearing you say is: "I'm worried that with the move of theme locations from a meta box to checkboxes, that after changing themes, I'll have to select each menu individually to set the theme location, whereas before I could set all of the theme locations at once".

Is that an okay summary? Or have I missed something?

#22 @lessbloat
11 years ago

Just making a record that the last round of user tests uncovered a bug. The advanced options in screen settings are unchecked, but they still show up in the menu item edit area.

#23 in reply to: ↑ 21 ; follow-up: @chipbennett
11 years ago

Replying to lessbloat:

Replying to chipbennett:

To clarify, more or less, what I'm hearing you say is: "I'm worried that with the move of theme locations from a meta box to checkboxes, that after changing themes, I'll have to select each menu individually to set the theme location, whereas before I could set all of the theme locations at once".

Is that an okay summary? Or have I missed something?

Concerns:

  1. When assigning pre-defined custom menus to a Theme's defined Theme Locations, the process now requires considerably more steps, clicks, page refreshes, and time
  2. When looking at the "Select a menu" dropdown, there is no way to determine if all Theme locations have an associated custom menu assigned
  3. When editing a given menu, in the Theme Location field checkboxes, there is no indication that a given Theme Location already has a custom menu assigned to it
  4. With long-ish custom menus, the "Theme Locations" field is buried "beneath the fold", resulting in no initially visible UI for assigning the current menu to a Theme Location
  5. Overall, the page now seems to emphasize adding/editing custom menus, and seems to deemphasize assigning custom menus to Theme Locations, and the latter is arguably the more important role/task for Appearance -> Menus

#24 in reply to: ↑ 23 @lessbloat
11 years ago

Replying to chipbennett:

Let's move this discussion to a new ticket please (as this topic has nothing to do with this current ticket).

#25 @JerrySarcastic
11 years ago

  • Cc fittingsites@… added

#26 @DrewAPicture
11 years ago

23450.7.diff fixes the screen option hide/show toggles for other menu attributes.

@DrewAPicture
11 years ago

Screen options attribute toggles

@DrewAPicture
11 years ago

post_types ordering

#27 @DrewAPicture
11 years ago

23450.8.diff fixes the post type array reversal to always show pages at the top, regardless of whether there are custom post types present.

@DrewAPicture
11 years ago

New approach w/ do_menu_accordion_sections()

#28 @DrewAPicture
11 years ago

After some discussion in IRC tonight, I decided to try a new approach to this with 23450.9.diff

It introduces three new functions:

  1. do_menu_accordion_sections()
  2. add_menu_accordion_section()
  3. remove_menu_accordion_section()

The add|remove_menu_accordion_section() functions are really just wrappers for add|remove_meta_box() but the idea is to reinforce that though we're technically still working with meta boxes, we're actually adding accordion sections.

The naming convention is hardly set in stone, so that's open to suggestions. Maybe do_menu_sections(), add|remove_menu_sections()?

There are several big pros for taking this approach:

  • It reduces the number of affected lines by about half (-400 lines)
  • It reinforces the idea that we're working with accordion sections, rather than meta boxes
  • It's fully backward-compatible with the meta box API

On another note, I decided to reintroduce the ability to hide individual accordion sections via Screen Options. I felt that since we've removed the ability to re-sort the sections, some people would appreciate (and expect) the ability to hide sections they don't use.

Next Steps:

23450.9 is a first pass at the new approach and we'll need to work out the kinks in terms of adding back the proper selectors to properly hide accordion sections. Feedback is welcome and encouraged.

@lessbloat
11 years ago

#29 @lessbloat
11 years ago

23450.10.diff should fix the hiding/showing & saving of the accordion menu item options.

#30 follow-up: @DrewAPicture
11 years ago

23450.10.diff works great!

Last night I also noticed a problem where switching tabs in the accordion sections results in empty tabs. JS is hardly my forte so I can't be sure if it is JS/selector or PHP related.

To reproduce:

  1. Apply 23450.10.diff
  2. On Appearance > Menus, try changing tabs in one of the accordion sections
  3. The tab will switch but it is empty
  4. Change back to the section's default tab, it's now empty.

This issue was introduced in 23450.9.diff.

@lessbloat
11 years ago

#31 in reply to: ↑ 30 @lessbloat
11 years ago

Replying to DrewAPicture:

23450.10.diff works great!

Last night I also noticed a problem where switching tabs in the accordion sections results in empty tabs.

23450.11.diff​ should do the trick.

#32 @DrewAPicture
11 years ago

23450.11.diff fixes the tabs issue for me. I think we're ready for another pre-commit review.

Also, free plugin idea: Make the accordion stick when scrolling for long menus. For instance, this is one of my test menus: http://cl.ly/image/04393K0z2y36

@DrewAPicture
11 years ago

Removes add|remove wrappers, renames do_accordion_sections()

#33 @DrewAPicture
11 years ago

23450.12.diff removes the unecessary add|remove_meta_box() accordion wrappers and renames do_menu_accordion_sections() to just do_accordion_sections().

The idea is that do_accordion_sections() could be used to turn any group of meta boxes into an accordion in the spirit of abstracting the accordion code away from the Customizer in #23449 in the first place.

#34 @DrewAPicture
11 years ago

As of 23450.12.diff, the menus accordion UI looks like this:
http://f.cl.ly/items/0P3x1S2a0C1B0u373o1v/36menus_preview.png

Last edited 11 years ago by DrewAPicture (previous) (diff)

#35 @DrewAPicture
11 years ago

  • Keywords commit added; needs-testing removed

@DrewAPicture
11 years ago

Minor cleanup, move outer accordion markup into do_accordion_sections() as with do_meta_boxes()

#36 @markjaquith
11 years ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In 23707:

Turn the Nav Menu meta boxes into an accordion. Less sprawling and overwhelming.

  • Registration stays the same — they're meta boxes
  • Call do_accordion_sections() instead of do_meta_boxes() and they render as an accordion

props DrewAPicture, lessbloat, jkudish. fixes #23450. see #23449

#37 @DrewAPicture
11 years ago

  • Keywords needs-codex added

#38 @DrewAPicture
11 years ago

For reference:

Discussion in IRC on 3/14 about whether to retain sortable functionality of accordion sections/meta boxes, also whether to just piggyback on do_meta_boxes() instead of duplicating code in do_accordion_sections() introduced in [23707].

#39 follow-up: @nacin
11 years ago

DrewAPicture's comment was the result of this comment on #23449:

I can't quite tell how much code is duplicated between do_accordion_sections() and do_meta_boxes(), but it seems like quite a bit. (And this code is, shall we say, unique, so keeping it DRY has some importance.) Could we just add an $args array to do_meta_boxes() for 'accordion' => true?

It seems like having $args including sortable, accordion, etc., as flags could be a nice-to-have. (Though, these flags in this single function may not be enough to actually make things work, which makes them lame.)

I will mention that 'sorted' is used in do_accordion_sections() as a priority to look at — but this must stay (and should be probably commented) in case do_meta_boxes() has done sorting on that page in some other situation.

Sorting, I'm being told in IRC, was left out because we wanted to enforce 'Pages' at the top. But, that can be done by using the meta box priority of 'high', I'm pretty sure.

#40 @nacin
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#41 in reply to: ↑ 39 @DrewAPicture
11 years ago

Replying to nacin:

Sorting, I'm being told in IRC, was left out because we wanted to enforce 'Pages' at the top. But, that can be done by using the meta box priority of 'high', I'm pretty sure.

Sorry if I wasn't clear. We didn't leave it out to enforce 'Pages' at the top, we left it out because we weren't sure we wanted to tackle trying to sort accordion sections in 3.6.

The 'Pages' not using the 'high' priority was an oversight leftover from previously using an array_reverse to bring it to the top of the pile. This was pre-23450.9.diff, pre-do_accordion_sections, pre-meta-boxes-back-compat.

#42 @nacin
11 years ago

  • Type changed from enhancement to task (blessed)

#43 @nacin
11 years ago

This also changed the classes on the customizer from customizer-* to accordion-*. This broke some special WordPress.com styles. I don't have a problem with doing this as long as WP.com is the only place this really affects — do we think that is the case?

@DrewAPicture
11 years ago

DRY-er approach

#44 @DrewAPicture
11 years ago

@nacin, got some red for you.

23450.14.diff takes a DRY-er approach and leaves do_accordion_sections() as a wrapper for do_meta_boxes().

#45 @DrewAPicture
11 years ago

Is there still a problem with the customizer classes?

#46 @ocean90
11 years ago

  • Keywords commit removed

#47 @nacin
11 years ago

So:

  • Let's ignore the customizer classes.
  • I don't think we need to DRY this for now. It ends up less readable, and it's late in the cycle to refactor.

I think the only thing we need to worry about is:

  • I will mention that 'sorted' is used in do_accordion_sections() as a priority to look at — but this must stay (and should be probably commented) in case do_meta_boxes() has done sorting on that page in some other situation.

As in, let's make sure the functions share the same code with regards to sorting.

#48 @nacin
11 years ago

Since my comment is confusing even to me, some clarifications:

  • Don't worry about the class name changes.
  • Don't try to combine the functions.

But, this code is missing: https://core.trac.wordpress.org/browser/trunk/wp-admin/includes/template.php?rev=24593#L898. Let's make sure that makes it in.

#49 @nacin
11 years ago

However: The boxes were sortable in 3.5. If we suddenly added back the sorting in case someone implements draggable accordions, what would actually happen is 3.5 -> 3.6 users would get their menus 'stuck'.

Per IRC, we should just leave this alone for 3.6. I'm removing 'sorted' from the loop.

#50 @nacin
11 years ago

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

In 24674:

Remove 'sorted' from do_accordion_sections(). fixes #23450.

Note: See TracTickets for help on using tickets.