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

Try showing plugin notices above the editor #4811

Merged
merged 3 commits into from Feb 22, 2018

Conversation

Projects
None yet
7 participants
@jasmussen
Contributor

jasmussen commented Feb 1, 2018

These notices aren't pretty. But now they're visible, so you kind of have to deal with them before you can edit. This is one approach we can take — the other being hide them and let the user deal with them on another screen in the wp-admin than the editor screen.

Thoughts? What's the best approach? Are there other approaches?

Screenshot:

screen shot 2018-02-01 at 18 57 39

Try showing plugin notices above the editor
These notices aren't pretty. But now they're visible, so you kind of have to deal with them before you can edit. This is one approach we can take — the other being _hide them_ and let the user deal with them on another screen in the wp-admin than the editor screen.

Thoughts? What's the best approach?
@StaggerLeee

This comment has been minimized.

StaggerLeee commented Feb 1, 2018

Same as Windows 10 notifications. Bottom-right corner, modal box semi-transparent, or similar. Timed, to disappear after X seconds.

@spencerfinnell

This comment has been minimized.

spencerfinnell commented Feb 1, 2018

Personally I've seen too many notices that aren't dismissible to think they should be shown above the editor and blocking inputs -- for that reason I would vote leaving them for another screen.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 2, 2018

Personally I've seen too many notices that aren't dismissible to think they should be shown above the editor and blocking inputs -- for that reason I would vote leaving them for another screen.

That's a good point.

Also a point that sort of deserves more opinions, so I'll let this PR sit for a bit.

@karmatosed

This comment has been minimized.

Member

karmatosed commented Feb 5, 2018

In principle I have an issue with non-dismissable notices. They are dark patterns and any plugin author should not be using them. That idealist position though doesn't help those users facing plugin authors using them today, sadly.

We have to make them at least work in Gutenberg. This said, I am slightly reluctant we add yet another message style to them. This feels like a new one. It could be the shadowing making this feel 'yet another' though.

I think this points to a deeper issue I want to investigate myself. Right now we have a lot of different places and styling for messages. That all is a future path though, for now I think making sure the z-index is 'message first' works as a starting point.

@karmatosed

I'm not sold on styling but the index works. Added a comment.

@spencerfinnell

This comment has been minimized.

spencerfinnell commented Feb 5, 2018

Until 3.3 WooCommerce had a non dismissible notice on every page. woocommerce/woocommerce#18452

I think this will cause an issue for a lot of people if they are shown over inputs. Obviously all notices should be dismissible but that’s not really the case realistically.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 6, 2018

Spencer makes a good point, if there are undismissable notices, putting these above the editor UI breaks it. Given we can't position these notices in any other way than to use CSS, this makes me think we should not show notices at all on this page.

Given the fact that the notices are visible on every other page, perhaps that's okay? We also have an upgrade path for plugin notices, like the colored notices you see when you publish. What do you think?

@spencerfinnell

This comment has been minimized.

spencerfinnell commented Feb 6, 2018

Notices could be collected and output via Gutenberg's own system (like metaboxes) in a similar way that the WP Notification Center plugin does it:

https://plugins.trac.wordpress.org/browser/wp-notification-center/trunk/src/AdminNoticeHandler.php

@spencerfinnell

This comment has been minimized.

spencerfinnell commented Feb 6, 2018

If hidden with CSS I'd suggest using the same rules used on the About page of core:

https://github.com/WordPress/WordPress/blob/master/wp-admin/css/about.css#L34-L38

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 7, 2018

Thanks for that link, Spencer, the fact that there's precedence for hiding the notices in WordPress already, makes me think we should absolutely do it here. The argument being: you're in a writing/publishing context, and you shouldn't have to deal with any notices that aren't from the editor itself. Especially when the rest of the admin inundates you with them. Just my opinion — but what do you think, @karmatosed? The fact that there exist undismissable notices is a problem we'd have to address if we were to keep them.

@StaggerLeee

This comment has been minimized.

StaggerLeee commented Feb 7, 2018

@jasmussen

I never was much angry about non-dismissable notices, as about fact they do not have different and separate HTML class. To be able to hide them with own code in "admin_init".

Last case I remember was WP-SpamShield plugin. Admin Notice shown there forever, and developer by purpose did not attach any class whatsoever so you can target Notice and hide it.

As long you from core force plugin, and theme, developers to attach a original class to their Notices I do not think it is huge problem. Some kind of WP standardization.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 7, 2018

As long you from core force plugin, and theme, developers to attach a original class to their Notices I do not think it is huge problem. Some kind of WP standardization.

I do agree it would be nice to have some good standards for how good notices are built, but I think it's difficult to enforce.

However due to the way Gutenberg is built, I'm pretty sure the following CSS can be used to target any notice, even one without a class:

#wpbody-content > div:not( .gutenberg ):not( #screen-meta ) {

So if we want to hide all notices, we can. Unless the plugin developer is being particularly evil.

@spencerfinnell

This comment has been minimized.

spencerfinnell commented Feb 7, 2018

Some other discussion here: #3395

@karmatosed

This comment has been minimized.

Member

karmatosed commented Feb 7, 2018

I think enforcing good notice practices on plugins is nobel and something as a community we should do, but not something Gutenberg can or should try to solve. I would agree that hiding in this case is best.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 19, 2018

Okay in the latest push I changed it so notices are hidden in Gutenberg. Plugins can still use Gutenberg notices to show information that the user has to see while on this screen.

Screenshots:

screen shot 2018-02-19 at 08 33 34

screen shot 2018-02-19 at 08 33 43

@@ -13,6 +9,12 @@ body.gutenberg-editor-page {
padding-bottom: 0;
}
/* We hide legacy notices in Gutenberg, because they were not designed in a way that scaled well.
Plugins can use Gutenberg notices if they need to pass on information to the user when they are editing. */
#wpbody-content > div:not( .gutenberg ):not( #screen-meta ) {

This comment has been minimized.

@youknowriad

youknowriad Feb 20, 2018

Contributor

Don't you think it's a bit too aggressive to hide everything like that? can't we like target notices specifically?

This comment has been minimized.

@jasmussen

jasmussen Feb 20, 2018

Contributor

Is there a CSS class we could hook into, that every existing plugin utilizes? When I looked at this a while back it seemed like notices were a complete wild west where plugin devs could create their own undismissable and totally custom notices.

This comment has been minimized.

@youknowriad

youknowriad Feb 20, 2018

Contributor

Fine enough, This also could be considered legit since we don't want any "uncontrolled" UI element outside the gutenberg controlled div.

I was afraid this would break the modals, but it seems that the media modal is still working.

@nerrad

This comment has been minimized.

Contributor

nerrad commented Feb 20, 2018

I think this is a good approach. However, when this is merged in I think it'd be a good idea to have some documentation about using the Gutenberg notices. I looked and couldn't find any but I should be able to derive from the code how to use them.

That way, when the change-log has "Hide all native WP admin notices" you can also have "Read here about how to implement Gutenberg notices".

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 21, 2018

Added a README on how to add editor notices. @karmatosed ready for final review!

@jasmussen jasmussen merged commit add42d9 into master Feb 22, 2018

1 of 2 checks passed

codecov/project 34.42% (-3.78%) compared to a5ca791
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/fix-plugin-notices branch Feb 22, 2018

@@ -0,0 +1,10 @@
This component is used to implement notices in editor.

This comment has been minimized.

@aduth

aduth Feb 22, 2018

Member

Anything in "components" is not meant to be specific to the editor.

Currently, adding notices externally is not very intuitive, but I think we might look to leveraging the data module for notices to become an independent unit that can be manipulated externally (and reused on any future screens).

This comment has been minimized.

@jasmussen

jasmussen Feb 22, 2018

Contributor

Is there anything actionable I should do at this point, to improve the README for example?

This comment has been minimized.

@aduth

aduth Feb 22, 2018

Member

It's fine. The README can be updated when notices are refactored.

@StaggerLeee

This comment has been minimized.

StaggerLeee commented Feb 27, 2018

I just had to meddle with developer console to remove some Notices from updated plugins. It has to be prevented and disabled.

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