WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 5 weeks ago

#46830 accepted defect (bug)

When menu item removed form Customizer menu uncheck it's source item

Reported by: garrett-eclipse Owned by: audrasjb
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Customize Keywords: has-patch has-screenshots
Focuses: ui, javascript, administration Cc:
PR Number:

Description

Hello,

When using the Customizer menu the menu items that you add get checked off on the original panel. But when you remove those pages from the menu those items stay checked which can mislead users to think their page is still in the menu. It would be nice if once the last page for the item is removed from the menu it's relevant source menu item be unchecked.

Sorry I hope that wasn't confusing adding a screenshot.

Attachments (6)

Screen Shot 2019-04-08 at 1.37.43 AM.png (62.0 KB) - added by garrett-eclipse 7 months ago.
Added page is checked off
Screen Shot 2019-04-08 at 1.37.52 AM.png (80.2 KB) - added by garrett-eclipse 7 months ago.
Removed page leaves item checked off which is misleading and gives it almost a disabled state
46830.diff (2.6 KB) - added by donmhico 3 months ago.
5f08acd6255c1464847595ab922ae4d1.gif (411.8 KB) - added by garrett-eclipse 2 months ago.
Successful testing demonstration showing checks revert to + plus icons when all items removed.
46830.2.diff (2.6 KB) - added by donmhico 2 months ago.
Changed @see to @link.
6c61d051028ffdacff8e2ff758d4e9df.gif (897.3 KB) - added by audrasjb 6 weeks ago.
Works fine

Download all attachments as: .zip

Change History (15)

@garrett-eclipse
7 months ago

Added page is checked off

@garrett-eclipse
7 months ago

Removed page leaves item checked off which is misleading and gives it almost a disabled state

#1 @audrasjb
7 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to audrasjb
  • Status changed from new to accepted

Hi and thanks for the ticket @garrett-eclipse

I can reproduce the issue on my side. Moving this one to the next release.

@donmhico
3 months ago

#2 @donmhico
3 months ago

  • Keywords has-patch added; needs-patch removed

@garrett-eclipse @audrasjb - If you have time, can you test and review my attached patch 46830.diff.

Thanks.

@garrett-eclipse
2 months ago

Successful testing demonstration showing checks revert to + plus icons when all items removed.

#3 @garrett-eclipse
2 months ago

  • Keywords has-screenshots added

Thanks @donmhico this worked really nicely, testing both Posts and Pages the check indicator reverts to the + add icon once all items are removed as desired.

Reviewing the code everything looked very well written thank you. In php unit tests I've seen ticket references as @ticket XXXXX but I'm not sure if that's also the standard with JS (referring to your @see reference).

@audrasjb would you mind taking a final review this works nicely and applies cleanly. I feel it's ready to commit but Customizer is your component ;)

P.S. While testing I found the check icon is lost if you click 'Add Items' to open/close the drawer. I've opened 47990 to look into keeping that check mark persistent.

Last edited 2 months ago by garrett-eclipse (previous) (diff)

#4 @donmhico
2 months ago

Thanks @garrett-eclipse. Maybe i'll use @link instead.

@see: A function or class relied on.
@link: URL that provides more information.

https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/javascript/

@donmhico
2 months ago

Changed @see to @link.

#5 @garrett-eclipse
2 months ago

Nice, good call thanks @donmhico

#6 @garrett-eclipse
6 weeks ago

  • Component changed from Menus to Customize
  • Focuses javascript administration added

Thanks again for the patch on this @donmhico it's still applying cleanly and manual tests are all working nicely.

@audrasjb can you give the patch a final review, I feel it's ready for commit but would prefer a maintainer to move it forward.

Cheers

#7 @audrasjb
6 weeks ago

  • Keywords commit added

Hi @garrett-eclipse,

The last patch applies cleanly and it works fine on my side after few local tests.
Looks good to go on my side.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


5 weeks ago

#9 @audrasjb
5 weeks ago

  • Keywords commit removed
  • Milestone changed from 5.3 to 5.4

Hi,
This ticket should have been committed before Beta 3 to ship with WP 5.3.
Moving it to milestone 5.4 as it's pretty close to a solution.

Note: See TracTickets for help on using tickets.