WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 4 weeks 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 needs-dev-note has-patch
Focuses: administration Cc:

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 12 months ago.
44941.2.diff (1.2 KB) - added by donmhico 6 weeks ago.
Removed the CSS part from 44941.diff and added the @since docs.
44941.3.diff (1.2 KB) - added by donmhico 6 weeks ago.
Corrected @since to adhere to WPCS.

Download all attachments as: .zip

Change History (17)

@desrosj
12 months ago

#1 @desrosj
12 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.


12 months ago

#3 @javorszky
12 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.


12 months ago

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


4 months ago

#6 @karmatosed
4 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
6 weeks ago

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

#7 @donmhico
6 weeks ago

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

@donmhico
6 weeks ago

Corrected @since to adhere to WPCS.

#8 @SergeyBiryukov
5 weeks ago

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

#9 @SergeyBiryukov
5 weeks 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
4 weeks 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
4 weeks 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
4 weeks 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
4 weeks ago

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

#14 @donmhico
4 weeks ago

  • Keywords has-patch added; needs-patch removed
Note: See TracTickets for help on using tickets.