WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 21 months ago

#37727 closed enhancement (fixed)

Allow for customize control notifications to have extensible templates

Reported by: westonruter Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.6
Component: Customize Keywords: has-patch
Focuses: javascript Cc:
PR Number:

Description

It was noted by @celloexpressions toward the end of 4.6:

FYI I just noticed an issue with the JS template for control notifications introduced in [37476]. We need to display the raw message rather than escaping it {{{ instead of {{ so that html is allowed. Which it should be for links, etc., and it's needed for #34923. I'm making the change in the next patch there and don't think we need to change it for 4.6 but wanted to point it out.

It is useful to embed non-message content into a notification to allow for buttons to action on the notification. However, I don't feel that just allowing the raw message to be injected into the notification template is necessarily the best way to go. To me it would seem better if we allowed for Notification subclasses that had custom templates to handle the various notification codes differently. Then the server wouldn't have to pass markup back as part of error messages.

Allowing custom templates for notifications would be highly useful for the global notification area in the Customizer (#35210).

See also https://core.trac.wordpress.org/ticket/30738#comment:16

Attachments (1)

37727.0.diff (2.0 KB) - added by westonruter 3 years ago.

Download all attachments as: .zip

Change History (12)

#1 @westonruter
3 years ago

@celloexpressions I just found a problem with adding markup to the message: screen readers could potentially read the markup. At least, that's what I just experienced with the wp-a11y-speech-synthesis plugin enabled.

@westonruter
3 years ago

#2 @westonruter
3 years ago

  • Keywords has-patch added; needs-patch removed

One approach is in 37727.0.diff where the Notification object can have a template argument which is then used to render the content of the notification (the containing li is not mutable here). Nevertheless, the entire template for rendering out all of the notifications can be overridden via the notificationsTemplate property on the wp.customize.Control instance itself. @celloexpressions any thoughts on this patch?

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


3 years ago

#4 @celloexpressions
3 years ago

  • Milestone changed from Awaiting Review to 4.7

It feels a bit odd to call it (and make it) a template since in most cases there won't/shouldn't be a need for any logic or even accessing the notification object. However, this should work well and also fixes an issue I'd seen with the message also going to screen readers. To avoid needing to make a separate message and template that are virtually the same, I wonder if the message could default to being the template with tags stripped if the message isn't provided but a template is?

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


3 years ago

#6 @celloexpressions
3 years ago

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

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


3 years ago

#8 @celloexpressions
3 years ago

  • Milestone changed from 4.7 to Future Release
  • Owner celloexpressions deleted

Per today's customize chat, we can safely switch to unescaped notification output, eliminating the immediate need for this. However, extensible templates might still be useful in some cases, so we'll reconsider in a future release.

#9 @westonruter
2 years ago

  • Milestone changed from Future Release to 4.9
  • Owner set to westonruter
  • Status changed from assigned to accepted

I'm going to resolve this with #35210.

#10 @westonruter
2 years ago

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

In 41374:

Customize: Add global notifications area.

  • Displays an error notification in the global area when a save attempt is rejected due to invalid settings. An error notification is also displayed when saving fails due to a network error or server error.
  • Introduces wp.customize.Notifications subclass of wp.customize.Values to contain instances of wp.customize.Notification and manage their rendering into a container.
  • Exposes the global notification area as wp.customize.notifications collection instance.
  • Updates the notifications object on Control to use Notifications rather than Values and to re-use the rendering logic from the former. The old Control#renderNotifications method is deprecated.
  • Allows notifications to be dismissed by instantiating them with a dismissible property.
  • Allows wp.customize.Notification to be extended with custom templates and render functions.
  • Triggers a removed event on wp.customize.Values instances _after_ a value has been removed from the collection.

Props delawski, westonruter, karmatosed, celloexpressions, Fab1en, melchoyce, Kelderic, afercia, adamsilverstein.
See #34893, #39896.
Fixes #35210, #31582, #37727, #37269.

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


21 months ago

Note: See TracTickets for help on using tickets.