WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 5 months ago

#37269 reviewing enhancement

Introduce removed event for wp.customize.Values collection

Reported by: westonruter Owned by: westonruter
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4
Component: Customize Keywords: has-patch has-unit-tests commit
Focuses: javascript Cc:

Description (last modified by westonruter)

Notifications were added to Customizer controls in #34893. Notifications are re-rendered when the control.notifications collection (a wp.customize.Values) has a wp.customize.Notification added or removed with the code as follows (via wp.customize.Control#initialize):

// After the control is embedded on the page, invoke the "ready" method.
control.deferred.embedded.done( function () {
        /*
         * Note that this debounced/deferred rendering is needed for two reasons:
         * 1) The 'remove' event is triggered just _before_ the notification is actually removed.
         * 2) Improve performance when adding/removing multiple notifications at a time.
         */
        var debouncedRenderNotifications = _.debounce( function renderNotifications() {
                control.renderNotifications();
        } );
        control.notifications.bind( 'add', function( notification ) {
                wp.a11y.speak( notification.message, 'assertive' );
                debouncedRenderNotifications();
        } );
        control.notifications.bind( 'remove', debouncedRenderNotifications );
        control.renderNotifications();

        control.ready();
});

Notice the multi-line comment. We can't re-render the notifications at a remove event because this event gets triggered just before it is removed, somewhat unexpectedly. We should therefore introduce a removed event that gets triggered after the removal so that workarounds using debounce or defer aren't required.

Submitted patch should include unit test.

Change History (10)

#1 @westonruter
9 months ago

  • Description modified (diff)
  • Keywords needs-unit-tests added

#2 @westonruter
7 months ago

  • Milestone changed from Future Release to 4.7
  • Version changed from 4.6 to 3.4

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


6 months ago

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


6 months ago

#5 @celloexpressions
6 months ago

  • Milestone changed from 4.7 to Future Release

Can still make 4.7 if there's a patch, but we need a patch for now.

#6 @delawski
5 months ago

@westonruter - What do you think of such implementation:

https://github.com/xwp/wordpress-develop/pull/193

#7 @delawski
5 months ago

  • Keywords has-patch added; needs-patch removed

#8 @westonruter
5 months ago

  • Keywords good-first-bug removed
  • Owner set to delawski
  • Status changed from new to assigned

@delawski nice. I added a code review comment.

#9 @delawski
5 months ago

@westonruter Thank you for the review. I've added the condition as you suggested and also wrote additional unit test to cover case you've mentioned in the comment.

#10 @westonruter
5 months ago

  • Keywords has-unit-tests commit added; needs-unit-tests removed
  • Owner changed from delawski to westonruter
  • Status changed from assigned to reviewing

Excellent. This can go into 4.8 as soon as trunk it open for enhancements.

Note: See TracTickets for help on using tickets.