WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 months ago

#31334 assigned enhancement

Customizer JS API should handle container removal from document

Reported by: Aniruddh Owned by: westonruter
Milestone: Future Release Priority: normal
Severity: normal Version: 4.1
Component: Customize Keywords: has-patch needs-refresh needs-unit-tests
Focuses: javascript, administration Cc:

Description

When we do

wp.customize.section.remove('my_section')

or

wp.customize.control.remove('my_control')

The API should handle removing their container from document, too. The current process requires you to execute this before removing a section (or control):

wp.customize.section('my_section').container.remove()

Attachments (4)

customize-base.js (15.6 KB) - added by Aniruddh 2 years ago.
Line 334-335
31334.diff (436 bytes) - added by Aniruddh 2 years ago.
Self patch added
31334_a.diff (507 bytes) - added by Aniruddh 2 years ago.
Sorry, I have less knowledge about how this whole thing works hence the previous dirty patch. I have updated the patch like you said and attached it here.
31334.2.diff (2.1 KB) - added by westonruter 2 years ago.
Define container removal in customize-controls.js, and use in customize-widgets.js

Download all attachments as: .zip

Change History (18)

@Aniruddh
2 years ago

Line 334-335

#1 @Aniruddh
2 years ago

  • Focuses javascript added

@Aniruddh
2 years ago

Self patch added

#2 @celloexpressions
2 years ago

  • Keywords has-patch needs-refresh added
  • Milestone changed from Awaiting Review to Future Release

@Aniruddh: can you update your patch to:

  • Use tabs instead of spaces for indentation
  • Add brackets around the if statement, per the coding standards (even though it's one-line)
  • Please patch relative to the root of WordPress, ie the patch should be something like src/wp-admin/js/customize-base.css

@westonruter: this looks good to me and makes sense for the API to handle, do you see any issues?

@Aniruddh
2 years ago

Sorry, I have less knowledge about how this whole thing works hence the previous dirty patch. I have updated the patch like you said and attached it here.

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


2 years ago

#4 @celloexpressions
2 years ago

  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 4.3
  • Owner set to westonruter
  • Status changed from new to assigned

Patch looks good, thanks. This'll go nicely with #30741.

@westonruter
2 years ago

Define container removal in customize-controls.js, and use in customize-widgets.js

#5 @westonruter
2 years ago

Yes, this makes sense. However, I think the logic shouldn't be added to the base class's method wp.customize.Values.prototype.remove() since there's nothing about container mentioned at all in customize-base.js.. Instead, this should be added in the subclasses that actually have this property. And then customize-widgets.js can also take advantage of this to remove its own control.container.remove(). See 31334.2.diff for what I have in mind.

Needs QUnit testing.

#6 @celloexpressions
23 months ago

@westonruter can you finish this off in the next day, or should we punt?

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


23 months ago

#8 @westonruter
23 months ago

  • Milestone changed from 4.3 to Future Release

I'm not going to have time, so yeah, punt.

#9 @celloexpressions
13 months ago

  • Keywords needs-refresh has-unit-tests added

Needs updates based on 5.

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


9 months ago

#11 @westonruter
9 months ago

As discussed in Slack, we may need to use jQuery#detatch instead of jQuery#remove, since remove will destroy added event listeners, causing the control to be inert if it is attempted to be re-added after it is removed.

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


2 months ago

#14 @westonruter
2 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

This needs to be revisited. If someone wants to refresh the patch and look further into any complications.

One thing to note: if a control gets removed from wp.customize.control then it should probably be forbidden from being re-added, the deferreds will have already resolved and any attached event listeners would have been destroyed. That is, unless we use jQuery#detatch instead of jQuery#remove.

Note: See TracTickets for help on using tickets.