Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#34755 closed defect (bug) (fixed)

"(Currently set to: %s)" string should use the location name instead of slug

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

Description

Background: #33431, [35722].

On the old Menus screen, the string is used for menu locations, and %s is a menu name.

In the Customizer, the same string is used for menus, and %s is a location slug. It should be the location name.

Attachments (3)

34755.diff (1.8 KB) - added by ryankienstra 9 years ago.
77813f8.diff (2.2 KB) - added by ryankienstra 9 years ago.
a9e1d83.diff (2.7 KB) - added by ryankienstra 9 years ago.

Download all attachments as: .zip

Change History (11)

@ryankienstra
9 years ago

#1 @ryankienstra
9 years ago

  • Keywords has-patch added; needs-patch removed

Patch For Defect
Requesting Review

@SergeyBiryukov,
Attached is a patch, and an identical GitHub PR for this defect.

This changes the "Menu Location" string in Customizer menu controls to the location name. Before, this string was the location slug.

So this adds an object to the data passed to the Customizer : locationSlugMappedToName. And uses this to retrieve the location name.

This also changes the argument name from themeLocations to themeLocationSlugs. This doesn't change the argument, it only clarifies the fact that it consists of slugs.

#2 @westonruter
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Future Release to 4.5
  • Owner set to ryankienstra
  • Status changed from new to assigned
  • Version set to 4.3

I'm noticing a build failure for the pull request, namely in the qunit tests: https://travis-ci.org/xwp/wordpress-develop/jobs/109268978#L1141

It looks like the test fixtures need to be updated to include the locationSlugMappedToName.

I also leaved a minor comment on the PR: https://github.com/xwp/wordpress-develop/pull/141#discussion_r52860094

@ryankienstra
9 years ago

#3 @ryankienstra
9 years ago

Updated Patch With 2 Corrections
Issue Of Failing JUnit Test Remains

@westonruter,
Thanks for your comments on the pull request for this ticket. Please see the new patch, based on your feedback.

As you mentioned here, there is still a failing JUnit test. This is at tests/qunit/wp-admin/js/customize-nav-menus.js#L27. I'll work on this soon.

Last edited 9 years ago by ryankienstra (previous) (diff)

@ryankienstra
9 years ago

#4 @ryankienstra
9 years ago

New Patch That Fixes JUnit Test

@westonruter,
Please see this patch, which has passing JUnit tests. The pull request's Travis build now passes.

As you suggested, I fixed the failing JUnit test by adding the object locationSlugMappedToName to fixtures/customize-menus.js.

#5 @westonruter
9 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner changed from ryankienstra to westonruter
  • Status changed from assigned to accepted

Thanks!

#6 @ocean90
9 years ago

With the attached patch we can revert/combine the changes in [35722] to wp-admin/nav-menus.php and
wp-includes/class-wp-customize-nav-menus.php.

#7 @ocean90
9 years ago

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

In 36573:

Customizer: In nav menus show the location name instead of slug.

Props ryankienstra.
Fixes #34755.

#8 @ocean90
9 years ago

In 36574:

Add missing test changes for [36573].

See #34755.

Note: See TracTickets for help on using tickets.