Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#35273 closed defect (bug) (fixed)

Menus Screen Options: various fixes for the Menus Screen and the Customizer Menus Panel

Reported by: afercia's profile afercia Owned by: westonruter's profile westonruter
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Menus Keywords: has-screenshots has-patch commit
Focuses: ui, javascript Cc:

Description

Splitting this out from #35112, see specifically comment 7 and comment 15.

In the Customizer, api.Menus.MenusPanel.hidden returns undefined when clicking for the first time on the screen options checkboxes.

https://cldup.com/D0Dr9UINiY.png

Also, the first parameter of substring() should be an integer between 0 and the length of the string. Currently, it works just because when the argument is NaN, it is treated as 0 so 0 should be used in the first place.

Additionally, the title attribute checkbox ID doesn't match the related title attribute ID used in the Menus screen so changing the visibility of the Title Attribute field in one of the two screens won't have any effect on the second screen. These IDs come from the screen "columns" and they're different, just compare WP_Customize_Nav_Menus_Panel::wp_nav_menu_manage_columns() and wp_nav_menu_manage_columns() in /wp-admin/includes/nav-menu.php.

In nav-menu.php the title attribute column has an ID: 'title-attribute' and was added in [32691]. A few days later, the Customizer Menus was merged in core and that column has an ID: 'attr-title' see [32806].

Checkboxes are also in a different order, this doesn't break the functionality but wondering if they should be in the same order for consistency.

Basically, the two wp_nav_menu_manage_columns() are duplicate and should always use the same IDs, wondering if a proper solution would be avoiding duplicated code in the first place.

Note for committers: props to @Asgaros and @azaozz so far.

Change History (25)

@afercia
9 years ago

#1 @afercia
9 years ago

  • Keywords has-patch has-screenshots added

First pass, any thoughts more than welcome.

#2 @afercia
9 years ago

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

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


9 years ago

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


9 years ago

#5 @westonruter
9 years ago

@afercia The changes look good to me. In 35273.2.diff I've also removed the duplicated list, with the wp_nav_menu_manage_columns method now being deprecated in favor of the existing global function. See change here: https://github.com/xwp/wordpress-develop/commit/7e09725372ef12de8999f5386d66bf4d72ceb8ce

Maybe @celloexpressions could any additional insight here since this is from the original Menu Customizer feature plugin.

#6 @afercia
9 years ago

Thanks @westonruter. The only outstanding issue I see is when quickly clicking on many checkboxes, there's no guarantee which request will complete after the other ones. Any of them could be slower, for any reason, and take a longer time to complete.

So, users may end up in a situation like the one in the screenshot below, where on the Customizer the request with hidden:link-target,attr-title started before but completed after the following request with hidden:attr-title and when refreshing the Menus screen (or even the Customizer) the hidden ones will be link-target,attr-title:

https://cldup.com/2_bqeKD_RW.png

The 5 related requests:

https://cldup.com/ewVPIUjyrZ.png

Not sure what can be done here :)

#7 follow-up: @westonruter
9 years ago

  • Keywords needs-patch added; has-patch removed

@afercia good point. What needs to be done here is (1) debounce the Ajax requests, and (2) abort any existing Ajax request that is currently being made.

#8 @celloexpressions
9 years ago

In the menu customizer plugin, it basically tried to do it as simply as possible by using the existing core functions. Definitely worth rethinking it at this point.

The title attribute screen option was made optional and off by default early on in the menu customizer process (I believe in June 2014). So, by the time that was made optional in core about a year later, the plugin had had that option well-established and I guess no one noticed that it was using a different id than the core one, or the other consistencies.

#9 in reply to: ↑ 7 @afercia
9 years ago

  • Owner changed from afercia to westonruter

Replying to westonruter:

@afercia good point. What needs to be done here is (1) debounce the Ajax requests, and (2) abort any existing Ajax request that is currently being made.

@westonruter thanks, not sure I have the skills (and the time) to look at this, going to assign it to you if you don't mind :)

#10 @westonruter
9 years ago

  • Status changed from assigned to accepted

#11 follow-up: @westonruter
9 years ago

@afercia please give 35273.3.diff a try. Note the changes to saveManageColumnsState, and how it will now both debounce requests with a 2-second delay, and any open Ajax request will get aborted if a new request comes in. The 2-second delay makes sense to me because this preference update only matters after they visit the Customizer the next time.

#12 @westonruter
9 years ago

  • Keywords has-patch added; needs-patch removed

#13 in reply to: ↑ 11 ; follow-up: @afercia
9 years ago

Replying to westonruter:

@afercia please give 35273.3.diff a try.

@westonruter 2 seconds seem a reasonable interval to me. Looks like it works nicely. Just 2 considerations:

  • should something similar be done also for the Screen Options in nav-menus.php (or for all the Screen Options auto-saving?)
  • please consider I really don't know anything about best practices to follow in WP for file inclusions, would including a whole file for just a single function be OK?

#14 in reply to: ↑ 13 @westonruter
9 years ago

  • Keywords needs-patch added; has-patch removed

Replying to afercia:

  • should something similar be done also for the Screen Options in nav-menus.php (or for all the Screen Options auto-saving?)

Yes. I'll have to let someone else come up with the patch for that.

  • please consider I really don't know anything about best practices to follow in WP for file inclusions, would including a whole file for just a single function be OK?

I would say yes. Better to eliminate redundancy than to load a bit more code.

#15 @westonruter
9 years ago

  • Owner westonruter deleted
  • Status changed from accepted to assigned

#16 @kirasong
9 years ago

  • Owner set to afercia

#17 follow-up: @afercia
9 years ago

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

About release schedule: I'd propose 35273.3.diff for commit consideration because it is an improvement. Then maybe split out the part about the Screen Options in nav-menus.php in a separate ticket.

#18 @westonruter
9 years ago

  • Owner changed from afercia to westonruter
  • Status changed from assigned to accepted

OK. Let's do that.

#19 @westonruter
9 years ago

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

In 36908:

Customize: Fix persistence of toggles for displayed nav menu item properties.

  • Eliminates need to click more than one checkbox to have preferences saved.
  • Adds debouncing to saving user-selected menu item properties.
  • Also removes discrepancies between available nav menu item properties on admin page vs Customizer.

Fixes #35273.
Props afercia, westonruter.

#20 in reply to: ↑ 17 ; follow-up: @westonruter
9 years ago

Replying to afercia:

Then maybe split out the part about the Screen Options in nav-menus.php in a separate ticket.

Over to you :)

#21 in reply to: ↑ 20 @afercia
9 years ago

Replying to westonruter:

Then maybe split out the part about the Screen Options in nav-menus.php in a separate ticket.

Over to you :)

Thanks :) Done, see #36202.

#22 @westonruter
9 years ago

In 37153:

Customize: Fix toggle of title attribute field visibility on nav menus admin page.

Improves alignment with nav menus in the Customizer, fixing regression introduced in [36908].

See #35273.
Props sidati, westonruter.
Fixes #36353.

Note: See TracTickets for help on using tickets.