Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#32820 closed defect (bug) (fixed)

Disambiguate "Automatically add new top-level pages to this menu." as being an option and not a menu location

Reported by: pbearne's profile pbearne Owned by: westonruter's profile westonruter
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch
Focuses: javascript Cc:

Description

Hi all

A small UI patch

In the new customizer the option to "Automatically add new top-level pages to this menu." looks like its part of the menu location list.

I have added a patch to give this a title of "Menu Options" to separate it from the location list

Paul

Attachments (6)

class-wp-customize-control.php.patch (530 bytes) - added by pbearne 10 years ago.
Patch to add "menu options" title
Capture.PNG (16.3 KB) - added by pbearne 10 years ago.
The menu without the Menu Options
Capture.2.PNG (7.1 KB) - added by pbearne 10 years ago.
Screenshot with the "Menu Options" Title
32820.diff (6.5 KB) - added by valendesigns 10 years ago.
nexus 5 chrome-emulated screenshot.png (34.9 KB) - added by westonruter 10 years ago.
32820.2.diff (691 bytes) - added by valendesigns 10 years ago.

Download all attachments as: .zip

Change History (28)

@pbearne
10 years ago

Patch to add "menu options" title

This ticket was mentioned in Slack in #core-customize by pbearne. View the logs.


10 years ago

#2 @celloexpressions
10 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.3

+1. This previously had some spacing at least, but not currently. However, this also brings up that this needs to be a separate control, as it was previously, so that plugins can add controls before and after it or remove it.

@pbearne
10 years ago

The menu without the Menu Options

@pbearne
10 years ago

Screenshot with the "Menu Options" Title

#3 @westonruter
10 years ago

As noted in Slack, the auto_add property of the nav_menu setting can receive a separate control in the same way that a separate control has been added for the nav menu's name: https://github.com/xwp/wordpress-develop/blob/556ec207f12da71aca788a1d3d98b317c97407d9/src/wp-admin/js/customize-nav-menus.js#L1577-L1621

So a nav_menu[...][auto_add] control would be added with a linkage to the nav_menu[…] setting, but the element would just sync with the auto_add property.

#4 @westonruter
10 years ago

  • Summary changed from it is not clear that "Automatically add new top-level pages to this menu." is na option and not a menu location to Disambiguate "Automatically add new top-level pages to this menu." as being an option and not a menu location

#5 follow-up: @pbearne
10 years ago

@afercia should this have an aria tag?

#6 in reply to: ↑ 5 @afercia
10 years ago

Replying to pbearne:

@afercia should this have an aria tag?

@pbearne hi, I think no. Have something specific in mind? I think it needs something more semantic than a span, though. Looking at this, the menu locations checkboxes are logically grouped so maybe "Menu locations" should be a legend of a fieldset that groups the checkboxes.
About the "Automatically add..." checkbox, it's a bit difficult since it's just 1 checkbox and there's nothing to actually "group" together. Agreed that should be separated from menu locations options since logically has a very different meaning.

#7 @valendesigns
10 years ago

  • Owner set to valendesigns
  • Status changed from new to assigned

I can work up a patch that adds a new control for auto_add.

#8 @obenland
10 years ago

@valendesigns any news?

@valendesigns
10 years ago

#9 @valendesigns
10 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Patch 32820.diff adds a new nav_menu_auto_add control to the end of the menu and I've tested that it actually adds top level pages. If there are any text changes that need to be made let me know. As it is now the new control looks like Capture.2.PNG.

#10 @westonruter
10 years ago

  • Owner changed from valendesigns to westonruter
  • Status changed from assigned to reviewing

#11 @celloexpressions
10 years ago

Patch looks good to me (untested though). My only suggestion would be to add a reference to #30741 in the todo comment about the control container code.

#12 @westonruter
10 years ago

  • Focuses javascript added
  • Keywords commit added; needs-testing removed

#13 @westonruter
10 years ago

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

In 33189:

Customizer: Disambiguate a menu's auto-add pages option from preceding menu location checkboxes.

Creates a separate nav_menu_auto_add control type following the pattern of the nav_menu_name control type.

Props valendesigns.
Fixes #32820.

#14 @ocean90
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

<span class="customize-control-title"><?php _e( 'Menu Options' ); ?></span> shouldn't be inside the label element and we should capitalize locations in Menu locations too.

Not sure about making Menu Options a part of the new control since you can remove and add more options, see comment:2. Thoughts?

#15 @valendesigns
10 years ago

I'm not exactly partial to that title either, as it's to ambiguous. Though If we remove the title completely nothing was achieved visually. Things would look exactly the same as before so maybe we parity the Admin Menus a bit more and use 'Auto add pages'. So we get some visual separation here.

If we go one step further and give the same treatment to add & delete in the section above. Then we could order it title, items, add, auto add, locations, delete and then we would more closely be mimicking the UI in Admin Menus.

Or should we just change the title to 'Auto add pages' and call it good?

#16 @pbearne
10 years ago

Part of the thinking to breaking this out was to allow "other options" to be added to the new control via action and filters

Otherwise, we could of stay with the just adding titles.

So I guess the question is is it possible to make the control extentable?

Paul

#17 @valendesigns
10 years ago

  • Keywords needs-patch added; has-patch commit removed

@pbearne Where in the description or comments does it mention or imply this issue has anything to do with actions and filters being applied inside the control?

#18 @celloexpressions
10 years ago

@pbearne the point of making this a distinct control is that it offers more flexibility with manipulating the UI here via the Customizer API than you could get via traditional actions and filters. See WP_Customize_Manager::add_control(), get_control(), and remove_control() for ways to add additional controls here, modify the auto add control, or remove the auto add control.

I think it's fine to say menu options by default, and plugins can remove/change that if they need to. A plugin that adds another basic checkbox control below the auto-add option would work perfectly with the current setup, though. I also think @nacin would probably prefer that we use Menu locations and Menu options for the capitalization, although I'd say it works either way as long as they're consistent.

This ticket was mentioned in Slack in #core-flow by boren. View the logs.


10 years ago

#20 @valendesigns
10 years ago

  • Keywords has-patch added; needs-patch removed

Moved the title out of the label and standardized the capitalization in 32820.2.diff.

This ticket was mentioned in Slack in #core-customize by valendesigns. View the logs.


10 years ago

#22 @westonruter
10 years ago

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

In 33317:

Customizer: Move "Menu options" heading outside of a nav menu auto-add control's label.

Standardizes capitalization. Amends [33189].

Props valendesigns.
Fixes #32820.

Note: See TracTickets for help on using tickets.