Opened 12 years ago
Closed 11 years ago
#23450 closed task (blessed) (fixed)
Refactor menu item meta boxes as accordion
Reported by: | lessbloat | Owned by: | 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)
Change History (67)
#5
@
12 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)
#6
@
12 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:
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...
#7
follow-up:
↓ 9
@
12 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?
#9
in reply to:
↑ 7
@
12 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.
#11
@
12 years ago
- Keywords needs-refresh removed
- Type changed from defect (bug) to enhancement
- Version set to trunk
#12
@
12 years ago
Testing 23450.2.diff:
- The accordion loads fully-folded for me: http://cl.ly/image/3Q1U263R1d0R
- Whichever fold of the accordion was open on last save should stick.
- Screen Options are gone
#13
@
12 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)
#14
@
12 years ago
2 small tweaks in 23450.5.diff:
- Moves Pages to the top
- Auto opens the top menu items accordion option
#16
@
12 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:
↓ 18
@
12 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
@
12 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.
#19
follow-up:
↓ 21
@
12 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.
#21
in reply to:
↑ 19
;
follow-up:
↓ 23
@
12 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
@
12 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:
↓ 24
@
12 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:
- 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
- When looking at the "Select a menu" dropdown, there is no way to determine if all Theme locations have an associated custom menu assigned
- 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
- 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
- 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
@
12 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).
#26
@
12 years ago
23450.7.diff fixes the screen option hide/show toggles for other menu attributes.
#27
@
12 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.
#28
@
12 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:
do_menu_accordion_sections()
add_menu_accordion_section()
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.
#29
@
12 years ago
23450.10.diff should fix the hiding/showing & saving of the accordion menu item options.
#30
follow-up:
↓ 31
@
12 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:
- Apply 23450.10.diff
- On Appearance > Menus, try changing tabs in one of the accordion sections
- The tab will switch but it is empty
- Change back to the section's default tab, it's now empty.
This issue was introduced in 23450.9.diff.
#31
in reply to:
↑ 30
@
12 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
@
12 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
#33
@
12 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
@
12 years ago
As of 23450.12.diff, the menus accordion UI looks like this:
@
12 years ago
Minor cleanup, move outer accordion markup into do_accordion_sections()
as with do_meta_boxes()
#36
@
12 years ago
- Owner set to markjaquith
- Resolution set to fixed
- Status changed from new to closed
In 23707:
#38
@
12 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:
↓ 41
@
12 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.
#41
in reply to:
↑ 39
@
12 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.
#43
@
12 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?
#44
@
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()
.
#47
@
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
@
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
@
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.
Sorry, 23450.diff was uploaded by mistake... It was supposed to be uploaded to #23449.