Skip to content
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

Capture admin notices and summarize them in editor #5590

Closed
wants to merge 2 commits into from
Closed

Capture admin notices and summarize them in editor #5590

wants to merge 2 commits into from

Conversation

coderkevin
Copy link
Contributor

@coderkevin coderkevin commented Mar 13, 2018

Description

This surrounds all admin notices with a containing div that then hides
them, and shows a count of admin notices rendered above the editor
content.

How Has This Been Tested?

Test with any admin notices that normally show up on a classic post edit. This will instead render a summary notice at the top of the post with a link to the dashboard, which displays admin notices normally.

I made a very big, annoying admin notice just to test this. It does everything in CSS that an admin notice shouldn't and therefore makes a good test: https://github.com/coderkevin/annoying-admin-notice

Screenshots (jpeg or gifs if applicable):

new_gutenberg_post___ _woo-test_ _wordpress

Types of changes

This PR does two things for admin notices:

Bug fix: Wraps all admin notices in a containing div, so they can be more effectively hidden.

New feature: Uses createInfoNotice to display a summary notice to show the user that there are WordPress notifications in a more pleasing way, with a convenient link to the dashboard for more information.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@jasmussen
Copy link
Contributor

This is really, really really cool, thanks so much for working on this.

screen shot 2018-03-14 at 09 25 50

Could you use an actual Gutenberg notice to display this message? Like this?

screen shot 2018-03-14 at 09 26 10

I believe that should make the notices stack and align properly, and then we can even make it dismissible.

Perhaps @aduth or @gziolo could help here?

@coderkevin
Copy link
Contributor Author

Could you use an actual Gutenberg notice to display this message? Like this?

I probably could. I'll look up how to do it. Do you want it dismissible like that one? Or should it stick around?

@jasmussen
Copy link
Contributor

Definitely dismissible. But if you don't have bandwidth to do this, feel free to bow out. Your work so far has been super appreciated, someone else from the team could presumably take your work and run with it.

@coderkevin
Copy link
Contributor Author

It's no problem, @jasmussen. I'll update the PR. 😄 I'd like to start getting some commits here anyway.

@danielbachhuber
Copy link
Member

with a convenient link to the dashboard for more information.

What happens if the notice only appears on the Post screen?

@coderkevin
Copy link
Contributor Author

What happens if the notice only appears on the Post screen?

That's a good question @danielbachhuber. I was a little unsure where to redirect the user. A previous implementation allowed errors to be viewed in an expandable drawer, but I didn't feel like it would be appropriate to do that within the post editor notice. Do you have a suggestion as to what should be done?

@danielbachhuber
Copy link
Member

A previous implementation allowed errors to be viewed in an expandable drawer, but I didn't feel like it would be appropriate to do that within the post editor notice. Do you have a suggestion as to what should be done?

Ideally, I think we should have some formal API (and corresponding UX) for plugins/themes to register notifications to Gutenberg. Historically, admin notices have been "dump whatever HTML you want to a certain position on the page". It would be better if we had different notification types, and some formal API/UX that intelligently handled a dozen plugins registering admin notices, etc.

I recognize that what I suggest is a bit more work though. Maybe we could get someone from the design team to head up a mini project to this effect.

@jasmussen
Copy link
Contributor

Completely agree with you that a larger better API is necessary. Cc @hedgefield as I've seen rumblings of that from him.

In the meantime it would be nice to have a holdover solution. In that vein perhaps it's fine redirect to the dashboard? This is from the perspective that any plugin that shows editor specific notices, but doesn't show Gutenberg specific notices, would presumably work better in the classic editor (either through the plugin, or through the presence of a non compatible metabox), where these would still show up. Does that make sense?

Alternately, perhaps we should indeed look at collapsing the legacy notices in the field.

@danielbachhuber
Copy link
Member

In the meantime it would be nice to have a holdover solution. In that vein perhaps it's fine redirect to the dashboard?

Could be worthwhile to include it for a release as experimental and see what sort of feedback we get.

This is from the perspective that any plugin that shows editor specific notices, but doesn't show Gutenberg specific notices, would presumably work better in the classic editor (either through the plugin, or through the presence of a non compatible metabox), where these would still show up. Does that make sense?

Maybe, although I'm not sure this is the majority use-case. Unknown at this point, I think.

@hedgefield
Copy link

Great work so far! Thanks for the ping @jasmussen, I did start a trac ticket to get a notification center into WP core recently https://core.trac.wordpress.org/ticket/43484 it's intended to completely capture and replace existing admin notices (except maybe from WP core), so you would indeed have a template to put your notice in instead of HTML whatever you want. Would be helpful in this usecase too I bet. Anyway It's just a proof of concept right now, and knowing core, will take a while to crystallize out ;) but it's good to keep in mind here, and any ideas generated here for something like that would be much appreciated. I'd like to get it going in as small a form as possible as soon as possible, so feel free to chime in on the trac ticket or contact me on slack.

As a holdover and Gutenberg-specific solution, this seems quite good already.

This surrounds all admin notices with a containing div that then hides
them, and shows a count of admin notices rendered above the editor
content.
@coderkevin
Copy link
Contributor Author

Rebased.

This uses `createInfoNotice` instead of rendering a `Notice` component.
@coderkevin
Copy link
Contributor Author

Could you use an actual Gutenberg notice to display this message? Like this?

@jasmussen, I've updated the code to use createInfoNotice for the notice. I've also updated the screenshot above. Thanks!

@jasmussen
Copy link
Contributor

This is what I see now, and I love it:

screen shot 2018-03-22 at 09 01 38

One thing, though, if I dismiss it then it's gone, cool. But if I reload the page, it's back again unless the notice has been dealt with elsewhere. This is probably fine, as presumably the notice is there for a reason, and we also don't want to have to build too much cookie logic into this dismissed notice.

The one situation where this might be an issue, is when, as Daniel suggests, a plugin is showing a notice that exists ONLY in the editor. Given the redirect you can never actually dismiss that.

I don't know how possible this is — but can we toggle a CSS class on #admin-notice-list when you click the link in the blue notice? You could then see something like this — ugly I know:

screen shot 2018-03-22 at 09 06 58

Perhaps with a close button in the top right corner that would untoggle the CSS class on #admin-notice-list again. I wrote this CSS in the inspector:

    background: #edeff0;
    position: absolute;
    z-index: 9999999;
    width: 100%;
    padding: 20px;
    box-sizing: border-box;
    border-bottom: 1px solid #e3e4e7;
    box-shadow: 0 10px 10px rgba(0,0,0,.1);

CC: @youknowriad for ideas, as I know he's also thought of notices recently.

@coderkevin
Copy link
Contributor Author

can we toggle a CSS class on #admin-notice-list when you click the link in the blue notice?

In wc-synchrotron, we had an accordion open and close which rendered the admin notices when open, but I was unsure of the best way to implement that here with the top menu bar and the notice being in the post content below.

@jasmussen
Copy link
Contributor

In wc-synchrotron, we had an accordion open and close which rendered the admin notices when open, but I was unsure of the best way to implement that here with the top menu bar and the notice being in the post content below.

If we can put all of the notices inside such an accordion, I think it's okay to "pull a big drawer down" on top of Gutenberg, as mocked up above, so long as you can close it.

@danielbachhuber
Copy link
Member

#3964 is an example of admin_notices being used for post-specific messaging

@danielbachhuber
Copy link
Member

Ideally, I think we should have some formal API (and corresponding UX) for plugins/themes to register notifications to Gutenberg. Historically, admin notices have been "dump whatever HTML you want to a certain position on the page". It would be better if we had different notification types, and some formal API/UX that intelligently handled a dozen plugins registering admin notices, etc.

Created #5975 for this.

@karmatosed
Copy link
Member

If we can put all of the notices inside such an accordion, I think it's okay to "pull a big drawer down" on top of Gutenberg, as mocked up above, so long as you can close it.

I can see the idea of an accordion but I have to admit I have concerns over the usability of that. What other options are there? Any examples out there we can learn?

@coderkevin
Copy link
Contributor Author

I'm definitely open to design options here. I feel like I'm kind of stuck on this until we can reach a consensus on what should be done.

@karmatosed
Copy link
Member

@coderkevin rather than holding it up why don't we get what we have merged in and then iterate?

@danielbachhuber
Copy link
Member

The link is incorrect. The notices should expand inline, instead of linking to the dashboard.

@coderkevin
Copy link
Contributor Author

Are we okay with expanding the notices inline? A sort of accordion that expands to show the notices in it? If so, I'll give it a try here.

@jasmussen
Copy link
Contributor

Are we okay with expanding the notices inline? A sort of accordion that expands to show the notices in it? If so, I'll give it a try here.

Yep, that would be totally fine. You can even do it in a half-hearted way and I'll polish up the visuals. Just ping me if you have a toggle that works.

@coderkevin
Copy link
Contributor Author

Okay, I'll add it to my to-do list this week. 👍

@aduth
Copy link
Member

aduth commented Sep 13, 2018

Is it likely this will be resumed, or can it be closed?

@aduth aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 13, 2018
@coderkevin
Copy link
Contributor Author

Is it still needed? If so, I can put some time towards it.

@aduth
Copy link
Member

aduth commented Sep 13, 2018

It might be safe to consider as partially blocked by #9617

@catehstn
Copy link

catehstn commented Nov 7, 2018

@coderkevin I see #9617 is now merged, so @aduth is it worth Kevin finishing this now?

@jasmussen
Copy link
Contributor

I personally would love to see this feature wrapped and shipped.

@aduth
Copy link
Member

aduth commented Nov 7, 2018

This would no longer be considered blocked by #9617, correct.

With that having been merged, it could have an impact on how best this be implemented (i.e. does it really make sense to be specific to the post editor, if it's more to the general issue of respecting server-side notices?).

There was some discussion yesterday in Slack (cc @desrosj) about this, and a point was raised that there's nothing which enforces that what is echo'd during the notices hooks actually be a notice (link requires registration):

https://wordpress.slack.com/archives/C02QB2JS7/p1541519233726300

(Mentioned also at #5590 (comment))

What we have here in inspecting class names to determine whether an element can be decided to be a notice seems like a reasonable enough compromise to cover the large majority of backwards-compatibility.

To simplify both technical and design implementation, I'm not sure why these can't just be treated as any other notice in the editor, rather than having their own standalone display.

I'm not sure about @coderkevin 's availability. I might also be able to put some time toward this.

@aduth
Copy link
Member

aduth commented Nov 7, 2018

Going to close this as superseded by #11604

@aduth aduth closed this Nov 7, 2018
@coderkevin coderkevin deleted the try/summarizing-admin-notices branch November 14, 2018 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants