New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compat: Upgrade admin notices to use Notices module at runtime #11604

Merged
merged 16 commits into from Nov 19, 2018

Conversation

Projects
None yet
8 participants
@aduth
Member

aduth commented Nov 7, 2018

Related: #5975, #6388
Supersedes #5590
Closes #10448

This pull request seeks to try to seamlessly upgrade notices output via an admin_notices or all_admin_notices action server-side.

Implementation notes:

The flow here is:

  • Discover admin notices by .notice class
  • Convert to @wordpress/notices notice object
    • This occurs in edit-post, though in the future I think either this compatibility should (a) become unnecessary or (b) be moved somewhere more generic
  • Dispatch to create notice through @wordpress/notices
  • Removes the admin notice from the DOM

Testing instructions:

Here's a plugin if you need one:

demo-notice.zip

This will add a dismissible success notice to any admin screen.

Verify that upon visiting Gutenberg, a notice echo'd during admin_notices or all_admin_notices is displayed.

Pending tasks:

  • Notice content is supported only as a plaintext string. Advanced markup is not supported. In not supporting non-plaintext, we can't truly close #10448, which includes an inline link. The notices module supports linkable "actions", but not interspersed with the plaintext. This ties in a bit with #9846, with the most direct solutions being one of either (a) allow elements as notice content or (b) pass notice content to a RawHTML component, neither of which is ideal in my mind.

@aduth aduth referenced this pull request Nov 7, 2018

Closed

Capture admin notices and summarize them in editor #5590

3 of 3 tasks complete
@mcsf

This comment has been minimized.

Contributor

mcsf commented Nov 8, 2018

Notice content is supported only as a plaintext string. Advanced markup is not supported. In not supporting non-plaintext, we can't truly close #10448, which includes an inline link. The notices module supports linkable "actions", but not interspersed with the plaintext. This ties in a bit with #9846, with the most direct solutions being one of either (a) allow elements as notice content or (b) pass notice content to a RawHTML component, neither of which is ideal in my mind.

Per Slack, I'd be OK with a compat-only way of providing raw content (__experimentalHTML), so something aligned with option b).

@aduth

This comment has been minimized.

Member

aduth commented Nov 8, 2018

@aduth aduth force-pushed the update/legacy-notices branch from d653835 to ab75260 Nov 8, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 8, 2018

I've updated the pull request to include support for the aforementioned __unstableHTML property on notices.

You can test with a revised example plugin, where the inline markup is now respected.

<?php

/**
 * Plugin name: Demo Notice
 */

function demo_notice_add() {
	echo '<div class="notice notice-success is-dismissible"><p>My <strong>notice</strong> text</p></div>';
}
add_action( 'admin_notices', 'demo_notice_add' );

Included are revisions to the coding standards document, as in discussing this with @nerrad (link requires registration), we came to agree that there's a subtle but important distinction between an experimental and an unstable API.

@aduth aduth force-pushed the update/legacy-notices branch from ab75260 to 35837cf Nov 8, 2018

@desrosj

This comment has been minimized.

Contributor

desrosj commented Nov 14, 2018

Sorry for the delay on this, @aduth. I have been traveling.

The functionality works great. Based on my testing, all notices output using the two specified filters that follow the current admin notice markup are converted to new Gutenberg notices correctly.

I tested this with all variations of supported notices, and they look great. Here is my slightly more exhaustive list: https://gist.github.com/desrosj/224863d7fb9fa9f4866ac5b2f6e15ebc

In testing the notice variations above, I did notice two issues, but I am not sure if they should be solved here or in a separate issue.

First, there are two types of notices currently supported in Core: .notice and .notice-alt. Currently, Gutenberg displays them the same.

Current admin notice styling:
screen shot 2018-11-13 at 10 22 55 pm

Admin notice styling in Gutenberg:
screen shot 2018-11-13 at 10 25 16 pm

Second, if admin notices are not dismissible, the editor can be rendered useless.
screen shot 2018-11-13 at 10 26 49 pm

It is not uncommon to have several notices in the admin, especially when the user has one or two plugins activated that have not yet been set up. I worry that this will render the editor unusable for some users. In the current state, any number of non-dismissible notices greater than one will hide the Title field.

Lastly, the order of the notices is reversed when converted. Is there any way to maintain the notice order? If a plugin uses an action priority of 999 to output their notice last, that behavior should be maintained.

@desrosj

This comment has been minimized.

Contributor

desrosj commented Nov 14, 2018

I did some more testing today. Turns out there are two additional instances I had missed for supported admin notices. The styles for these instances are missing.

<div class="error"><p>My error notice text.</p></div>
<div class="updated"><p>My updated notice text.</p></div>

Warning and info notices are not currently supported this way (with .warning .notice).

I also started to look at some other ways plugins are using these core notice patterns. I found a few instances where notices had multiple paragraph tags in one notice.

<div class="updated"><p>Your action succeeded!</p><p>Here is direction for what to do next.</p></div>

screen shot 2018-11-14 at 2 46 39 pm

Gutenberg equivalent:
screen shot 2018-11-14 at 2 47 26 pm

Only the first paragraph is moved over in these instances.

It's also not uncommon to have buttons in a second paragraph, such as with WooCommerce.
screen shot 2018-11-14 at 2 56 58 pm

In Gutenberg:
screen shot 2018-11-14 at 3 00 41 pm

I'm starting to worry that this one is more complex than we thought. I still believe that this should be present in 5.0, but it seems we may have to make decisions about what is and is not supported.

This exercise has also made me feel strongly that a dev note should be published on make.wordpress.org/core detailing how a plugin can ensure compatibility of their notices in Gutenberg while maintaining backward compatibility.

@aduth

This comment has been minimized.

Member

aduth commented Nov 14, 2018

First, there are two types of notices currently supported in Core: .notice and .notice-alt. Currently, Gutenberg displays them the same.

Forgive my ignorance: Can you explain or point me in a direction which explains what an "alt" notice is meant to be?

Second, if admin notices are not dismissible, the editor can be rendered useless.

This is good to note, and practically speaking could occur today in the editor. I think we'd either want to make it so that every notice must be dismissible, or that the notices are rendered in a way which pushes down the editor.

(could use some design feedback, cc @jasmussen ?)

Lastly, the order of the notices is reversed when converted. Is there any way to maintain the notice order?

I'm not positive on what basis it was reached, but the behavior of the notices implementation in Gutenberg is such that the notices which are added last are shown at the top of the list, which is consistent with what you've described.

The styles for these instances [.error, .updated] are missing.

Good catch. I suppose it would be good to add these. Tracking ad hoc variations from the core stylesheets is becoming a bit cumbersome 😕

https://github.com/WordPress/WordPress/blob/da7a80d67fea29c2badfc538bfc01c8a585f0cbe/wp-admin/css/common.css#L1344-L1346

I found a few instances where notices had multiple paragraph tags in one notice. [...] Only the first paragraph is moved over in these instances.

The reason I'd decided to target specifically the paragraph (wrongly assumed singular) previously was partly to avoid including the dismiss button. Maybe it would make more sense to explicitly target that for removal, and keep everything else intact verbatim.

@desrosj

This comment has been minimized.

Contributor

desrosj commented Nov 15, 2018

Forgive my ignorance: Can you explain or point me in a direction which explains what an "alt" notice is meant to be?

As far as I know, it is just an alternate styling available.

This is good to note, and practically speaking could occur today in the editor. I think we'd either want to make it so that every notice must be dismissible, or that the notices are rendered in a way which pushes down the editor.

It can happen in the editor today, but the notices will not overlap the editor, it will push the title field and editor down for better or worse. I think replicating that in Gutenberg would be fine but wonder about the implications of having to scroll down to get to the editor.

In my opinion, we should not make non-dismissible notices dismissible. Usually, the notice is there and not dismissible for a reason (a persistent reminder to the user to perform an action).

I'm not positive on what basis it was reached, but the behavior of the notices implementation in Gutenberg is such that the notices which are added last are shown at the top of the list, which is consistent with what you've described.

Gotcha. That makes sense, but I think for the maximum backward compatibility we should reverse the list before passing the notices to API.

Good catch. I suppose it would be good to add these. Tracking ad hoc variations from the core stylesheets is becoming a bit cumbersome 😕

I am pretty sure my Gist has an exhaustive list now. I also added .notice-title example which just has larger text and a few icons that are possible that I added for completeness.

The reason I'd decided to target specifically the paragraph (wrongly assumed singular) previously was partly to avoid including the dismiss button. Maybe it would make more sense to explicitly target that for removal, and keep everything else intact verbatim.

Reversing the logic makes sense. Let's see how that works out.

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

It can happen in the editor today, but the notices will not overlap the editor, it will push the title field and editor down for better or worse.

By today, I meant specifically with the notices module. For example, running this code in the console:

[ ...Array( 5 ) ].forEach( () => wp.data.dispatch( 'core/notices' ).createInfoNotice( 'hello', { isDismissible: false } ) )

The "push down" behavior might take effect for server notices, but not for the ones we render natively in the editor. I think it'd probably be sensible to implement it this way anyways, as the overlap seems like it'd almost always be the undesirable behavior. It has an impact on design though.

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

Forgive my ignorance: Can you explain or point me in a direction which explains what an "alt" notice is meant to be?

As far as I know, it is just an alternate styling available.

If it serves no semantic purpose, I'm inclined to think it should just be made unsupported in the transition.

I think the same would apply to .notice-large.

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

I am pretty sure my Gist has an exhaustive list now. I also added .notice-title example which just has larger text and a few icons that are possible that I added for completeness.

I totally missed this in your original comment. This is great to work from. Thanks!

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

I'm going to add the Needs Accessibility Feedback label because I'm concerned of two things:

  • Should we announce the adapted notices? I expect no such announcement occurs for any other server-rendered notice. Thus, maybe we need an option for notice creation to disable the speaking behavior, or at least disabling it for the notices compatibility. Or maybe wp.a11y can be enhanced to avoid announcing anything which occurs immediately at page load.
  • Even if we were to speak something, what can we assume programmatically to be spoken for a notice which might contain many paragraphs? I'd started down the path of something like paragraphs.map( ( p ) => p.textContent.trim() ).join( ' ' ) but it feels a bit fragile.

@aduth aduth force-pushed the update/legacy-notices branch from 35837cf to c308c3e Nov 16, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

I've rebased to resolve conflicts, and have pushed a number of revisions:

  • Account for .updated and .error notice classes (f9b44c5)
  • Use all markup of the notice (except the dismiss button) (234b58b)
  • Reverse order of server-rendered notices (c308c3e)

To the question of: "What should be spoken for notices?" The current state of the branch adds an option to disable spoken announcements for notices. (137b3bd) Upgraded notices will not be spoken. (234b58b)

To the question of: "What to do about overlapping undismissible notices?" The current state of the branch does nothing (yet). I posed the question in Slack (link requires registration) where there was some overwhelming feedback that all notices should be dismissible (until better broad support is implemented).

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

To the question of: "What to do about overlapping undismissible notices?" The current state of the branch does nothing (yet). I posed the question in Slack (link requires registration) where there was some overwhelming feedback that all notices should be dismissible (until better broad support is implemented).

Update: Continued discussion led to a different conclusion: Non-dismissible notices should remain non-dismissible, but should push down the rest of the editor such that they don't render it inoperable. Dismissible notices should remain floated above the editable area.

Terrible ASCII art:

+---------------------+
| Header              |
+----------------+----+
| Undismissible  |    |
+----------------+    |
| +------------+ |    |
| | Dismissible| |    |
| +------------+ |    |
|                |    |
|                |    |
|                |    |
|                |    |
+----------------+----+

I'm inclined to think that if agreeable, this should be done as a task separate to what's intended by the changes here, since it affects editor notices broadly irrespective of server-rendered compatibility.

desrosj added a commit to desrosj/gutenberg that referenced this pull request Nov 16, 2018

Display the privacy policy help notice with the `admin_notices` actio…
…n hook.

The new editor does not support the `edit_form_after_title` action hook. Because WordPress Core uses this hook to output the notice, it is not printed to the screen.

After WordPress#11604 is merged, legacy style admin notices (`<div class="notice">...notice content...</div>`) will be consumed by the Notices API and displayed. This change ensures that when WordPress#11604 is merged the privacy policy notice will appear again when editing the privacy policy page.

@desrosj desrosj referenced this pull request Nov 16, 2018

Merged

Fix the Privacy Policy help notice #11999

3 of 4 tasks complete
@desrosj

This comment has been minimized.

Contributor

desrosj commented Nov 16, 2018

Let me know when you are ready for a review, @aduth. Thanks for all the work on this!

One thing that I wanted to point out, though, is that #10448 is not closed by this issue without #11999. That notice is hooked to edit_form_after_title in WordPress core, which is not supported by the new editor. This means the notice is never output to the screen for the notices API to consume.

That PR will move it to admin_notices when the plugin is active so this PR can pass it into the Notices API.

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

I think it's ready for a proper review in its current form, with aforementioned caveat that we're not yet doing anything about undismissible notices blocking the editor.

The prior build failure was due to neglecting to rebuild the docs. Should hopefully be cleared up by e0960ff.

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

I'll remove the "Closes" signal from the original comment, to reflect the fact there is remaining work to be done.

@desrosj

This comment has been minimized.

Contributor

desrosj commented Nov 17, 2018

From a testing perspective, this looks great. A few notes:

  • Looks like the .notice-title styles carried over with no issues.
  • In the current state, the -alt and icon styles would not carry over.
  • The notices are now showing in the same order as output serverside.

I wonder if dismissible notices covering the editor fields will be confusing for users on new posts or empty editor screens.

I defer the code review to someone stronger in Javascript.

@aduth aduth force-pushed the update/legacy-notices branch from 130d5c4 to 5dd697d Nov 19, 2018

@aduth aduth force-pushed the update/legacy-notices branch from 5dd697d to 7e4cb9a Nov 19, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

Audible messages really need to be carefully crafted and use a dedicated string.

This was part of my motivation in only allowing plaintext strings for Notices, which is okay enough for our own usage, but in compatibility we can't do much about plugins which had used any arrangement of markup for their notices.

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

I've pushed a couple final revisions per feedback:

  • Only notices under #wpbody-content are upgraded, which is consistent with admin placement at point of actions firing
  • __unstableHTML is consolidated back to content with the flag being assigned as an option, thus reverting the overloaded form of the createNotice action to be potentially decided upon later.

As best I can gather, all other points pertinent to what's being considered here for compatibility have been addressed.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 19, 2018

Let's get this in for 4.5 and follow-up on any necessary tweaks.

@aduth aduth merged commit 5dbc641 into master Nov 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the update/legacy-notices branch Nov 19, 2018

@desrosj

This comment has been minimized.

Contributor

desrosj commented Nov 19, 2018

Realized the core use case for the alt notices just now and wanted to note it for later. The inline plugin and theme updates utilize these:

screen shot 2018-11-19 at 3 08 14 pm

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Fix the Privacy Policy help notice (WordPress#11999)
* Display the privacy policy help notice with the `admin_notices` action hook.

The new editor does not support the `edit_form_after_title` action hook. Because WordPress Core uses this hook to output the notice, it is not printed to the screen.

After WordPress#11604 is merged, legacy style admin notices (`<div class="notice">...notice content...</div>`) will be consumed by the Notices API and displayed. This change ensures that when WordPress#11604 is merged the privacy policy notice will appear again when editing the privacy policy page.

* Use `get_post()` instead of `global $post`.

* Add missing `@SInCE` documentation tag.

* Only display the notice if the function is actually hooked to `edit_form_after_title`.

This prevents the notice from displaying twice if the notice has already been moved to the `admin_notices` hook (as would happen in https://core.trac.wordpress.org/attachment/ticket/45057/45057.diff).

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Compat: Upgrade admin notices to use Notices module at runtime (WordP…
…ress#11604)

* Notices: Normalize notice type to use WP prefix

* Notices: Support Notice object as argument of createNotice

* Edit Post: Upgrade admin notices to notice module

* Notices: Extract default status to constant

* Docs: Add unstable API convention to coding guidelines

See: https://wordpress.slack.com/archives/C5UNMSU4R/p1541690775295000

(Registration: https://make.wordpress.org/chat/)

* Notices: Support __unstableHTML action property

* Components: Pass through __unstableHTML as Notice RawHTML

* Edit Post: Pass through notice HTML from admin notices

* Notices: Enforce string-y content by cast

* Notices: Add speak option

* Edit Post: Add missing AdminNotices classes

* Edit Post: Derive all AdminNotices upgraded notice children

* Edit Post: Fix AdminNotices missing text nodes content

* Edit Post: Reverse order of AdminNotice upgraded notices

* Notices: Mark as __unstableHTML via option

Content is used for both

* Edit Post: Limit upgraded notices to wpbody-content ID

@nerrad nerrad referenced this pull request Nov 24, 2018

Closed

Problem with admin_notice #12243

aduth added a commit that referenced this pull request Nov 29, 2018

@collimarco

This comment has been minimized.

collimarco commented Dec 7, 2018

I use Wordpress 5.0 and this feature doesn't work.

I have a plugin with this simple code:

function pushpad_notices() {
	echo '<div class="notice notice-success is-dismissible"><p>My notice text</p></div>';
}
add_action( 'admin_notices', 'pushpad_notices' );

The test notice is displayed properly on all admin pages, except for the editor page! I don't see any notice there. What should I do?

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Dec 7, 2018

@collimarco This was ultimately reverted in #12444 and restored to the original Gutenberg behaviour of hiding admin notices rendered through PHP. I hope in Phase 2 we can look at ways to improve this with a more reliable implementation!

@collimarco

This comment has been minimized.

collimarco commented Dec 7, 2018

@chrisvanpatten Thanks for the reply. However I don't understand what I am supposed to do in order to display a notice at the moment. Can you give me an example? Or should I keep using admin_notices and simply wait that you fix it?

@aduth

This comment has been minimized.

Member

aduth commented Dec 11, 2018

Hi @collimarco , for the moment the editor supports only notices created in JavaScript using the data module. More information about the data module (the wp-data script handle, made available at wp.data in the browser) and actions available specific to notices can be found at the following links:

In the future, there may be some patterns surrounding aggregating server-side notices for display in the editor, like discussed at the following links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment