WordPress.org

Make WordPress Core

Opened 14 months ago

Closed 3 months ago

Last modified 4 weeks ago

#44941 closed enhancement (fixed)

Support `warning` and `info` type notices in settings errors

Reported by: desrosj Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-screenshots has-patch has-dev-note
Focuses: administration Cc:
PR Number:

Description

The add_settings_error() only supports error and updated admin notices. These should also be supported for scenarios where error or updated are not appropriate.

Attachments (3)

44941.diff (1.4 KB) - added by desrosj 14 months ago.
44941.2.diff (1.2 KB) - added by donmhico 3 months ago.
Removed the CSS part from 44941.diff and added the @since docs.
44941.3.diff (1.2 KB) - added by donmhico 3 months ago.
Corrected @since to adhere to WPCS.

Download all attachments as: .zip

Change History (18)

@desrosj
14 months ago

#1 @desrosj
14 months ago

  • Keywords 2nd-opinion needs-testing added

44941.diff uses .settings-error.warning and .settings-error.info to add support for these. The updated and error notices are supported with the div.updated and div.error selectors, but adding div.warning and div.info to core would probably result in plugin and theme elements unintentionally being styled.

If the div selectors are preferred, that could maybe go into a major release if accompanied by a dev-note, but that doesn't address all backward compatibility concerns.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


14 months ago

#3 @javorszky
14 months ago

  • Keywords has-screenshots ui-feedback added; 2nd-opinion needs-testing removed

This looks good! Tried with

add_settings_error( 'bulk_action', 'bulk_action', 'Error text', 'info' );

and

add_settings_error( 'bulk_action', 'bulk_action', 'Error text', 'warning' );

They look like this:

https://d3vv6lp55qjaqc.cloudfront.net/items/0U1q2G3c0Z0C1Q1B0N1t/Screen%20Shot%202018-09-26%20at%2022.08.55.png

This ticket was mentioned in Slack in #core-privacy by javorszky. View the logs.


14 months ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


5 months ago

#6 @karmatosed
5 months ago

  • Keywords ui-feedback removed

This was discussed in this week's triage meeting. Thank you for the work done here, looks great and removing feedback keyword.

@donmhico
3 months ago

Removed the CSS part from 44941.diff and added the @since docs.

#7 @donmhico
3 months ago

In my patch 44941.2.diff, I removed the CSS part because notices should use the existing .notice-* styles. See #44640.

@donmhico
3 months ago

Corrected @since to adhere to WPCS.

#8 @SergeyBiryukov
3 months ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#9 @SergeyBiryukov
3 months ago

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

In 45814:

Administration: In add_settings_error(), add warning and info as possible values for message type.

Account for these new values in settings_errors(), resulting in notice-warning and notice-info CSS classes.

Map legacy error and updated CSS classes to notice-error and notice-success.

Props donmhico, toddhalfpenny, flixos90, desrosj, javorszky, SergeyBiryukov.
Fixes #44640, #44941.

#10 follow-up: @afercia
3 months ago

  • Keywords needs-dev-note added

Minor: worth noting the changes in [45814] break plugins that are (with good intent) passing a proper CSS class. For example, if a plugin passes notice-error, the CSS class printed out will now be notice-notice-error. Maybe worth considering a quick dev note. See also #44640.

Yes, the previous documentation mentioned Accepts 'error' or 'updated' but there was nothing in the code to prevent to pass arbitrary class names. Even in the new code there's nothing to prevent it.

Also, one could pass a string like the following:

error my-own-css-class hello world

and actually get the following (spaces are stripped out by sanitize_html_class()):

notice notice-errormy-own-css-classhelloworld settings-error is-dismissible

#11 in reply to: ↑ 10 @SergeyBiryukov
3 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to afercia:

Minor: worth noting the changes in [45814] break plugins that are (with good intent) passing a proper CSS class. For example, if a plugin passes notice-error, the CSS class printed out will now be notice-notice-error. Maybe worth considering a quick dev note. See also #44640.

Good catch, we should only add notice- if it doesn't already exist, and only to error, success, warning, info, not other random classes.

Also, one could pass a string like the following:

error my-own-css-class hello world

and actually get the following (spaces are stripped out by sanitize_html_class()):

notice notice-errormy-own-css-classhelloworld settings-error is-dismissible

I guess I was a bit overzealous with sanitize_html_class(), esc_attr() could be used instead.

Looks like this needs some more work, I don't think we should break backward compatibility here.

#12 @SergeyBiryukov
3 months ago

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

In 45873:

Administration: Adjust [45814] to address a backward compatibility issue for plugins passing multiple CSS classes to add_settings_error().

Only add the notice- prefix for error, success, warning, info CSS classes, keep other classes as is.

Add unit tests for settings_errors().

Props afercia, SergeyBiryukov.
Fixes #44941.

#13 @donmhico
3 months ago

Nice catch @afercia and thanks for the resolution @SergeyBiryukov.

#14 @donmhico
3 months ago

  • Keywords has-patch added; needs-patch removed

#15 @desrosj
4 weeks ago

  • Keywords has-dev-note added; needs-dev-note removed

Mentioned in the Miscellaneous Developer Focused Changes dev note for 5.3: https://make.wordpress.org/core/2019/10/15/miscellaneous-developer-focused-changes-in-5-3/

Note: See TracTickets for help on using tickets.