-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 using snackbar notices instead of prominent ones for saving/failure notices #15594
Conversation
Quick GIF: This is really nice! Aside from a little design polish — I'll help here — this is a really great first stab. A few quick thoughts, but I'll let others chime in as well:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this ui pattern so I think it'll be an improvement for certain types of notices. A couple thoughts though:
- In general the notices system in WordPress is in the need of an overhaul and I think GB is starting to establish new patterns for notices that could eventually replace what's currently in WP. I think some consideration needs to be given to how this works/looks for the rest of WP as a whole as we add these new patterns.
- I wonder if snackbar notices should be a bit more opinionated about what is allowed as content. Typically they are shorter snippets visually notifying of an action that just completed. So should there be some rules applied to not only what content is allowed in them, but also how much content?
- I wonder if notices created by WP/GB core should have a reserved style for them? Doing this would allow for easier distinction (both visually and accessibly) between what notices are added by WordPress/GB and what were added by a plugin.
} ) { | ||
const classes = classnames( className, 'components-snackbar', 'is-' + status ); | ||
|
||
if ( __unstableHTML ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long has __unstableHTML
been around and is this a pattern that should continue to be replicated in new things? Is there any attempts to remove the necessity for it being __unstable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question, I think we should do an audit of the experimental APIs and see which ones can be made "stable".
That said, I think __unstable
is a bit different in the sense that it's not meant to be made stable at all. It's something used internally that third-party developers shouldn't rely on. That doesn't mean the use-case here is not legit and shouldn't be exposed but it shouldn't in its current form. For this particular one @aduth knows more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case, it was added to support back-compat with the old notice system which allowed plugins to add arbitrary html (and was related to some work @aduth did with automatically porting usage of add_action( 'admin_notice' )
over to the notices in GB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just remove it, in that case as we don't support these admin notices anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at a minimum it should be removed/ignored for the new snackbar notices. I'm not comfortable making the call in general for all notices though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... this one's been on my mind lately.
It's not used anywhere in Gutenberg currently. It was originally part of some compatibility effort for server-rendered notices which was dropped late in the 5.0 release cycle. In fact, I still have an open pull request to remove it: #12440 . We would be safe in removing it.
This sat longer than I'd liked, and I know of several occurrences of people indicating they're using it, despite repeated warnings of its unstable nature. The last time it came up in a #core-js chat, we said that we didn't care, and that we'd still remove it since we're not obligated to the backwards-compatibility. It doesn't sit well with me though, largely on the point that we let this specific one linger, and thus it's more "on us".
My latest thought was to do a search in the plugins repository for who might be using it, and to both (a) consider their use case and (b) reach out to them directly ahead of its removal.
I ran the search now:
https://wpdirectory.net/search/01DAVD7SA20EYCQM5K456HMYNK
There aren't quite as many hits as I had expected, but certainly some popular ones.
I think regardless whether we decide to do it now (for which an argument could certainly be made), considering the use-cases is valid.
For example, one instance I'd heard that someone still chose to use __unstableHTML
was the need for dismissed notices to be persistent. This seems like a useful feature that we'd want to consider to implement anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should flag this in #core-js and also try reaching out to those using it to gather feedback on the use-case requirements for it? Then we could try to implement something that satisfies common use-cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should flag this in #core-js and also try reaching out to those using it to gather feedback on the use-case requirements for it? Then we could try to implement something that satisfies common use-cases.
I've added it to the agenda as an item to discuss:
https://docs.google.com/document/d/1h21-24QSi02QXr3qMqUAkzpT4B02Ep3ZKKI2qhgwUUw/edit
To piggyback off of Joen, some additional guidance:
|
I added a timeout, now these notices disappear after 10 seconds. |
} ) { | ||
useEffect( () => { | ||
// This rule doesn't account yet for React Hooks | ||
// eslint-disable-next-line @wordpress/react-no-unsafe-timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule doesn't account for React Hooks. It should be possible to use and cancel timeouts in useEffect or other hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked here #15622
These snackbar notifications are low-priority and contextual notifications. I think they defer a little bit from what would be a notification system in WordPress where notices could be persisted across pages and have persistent actions. That said, the patterns introduced here can easily be reused in different pages thanks to the "notices" package. |
So happy to see this happening! This is something we'll need for the navigation menu block as well, which we added in part thanks to @LukePettway's accessibility feedback. 🙌 Our initial mockup here uses the colour system of levels, but I'd be in agreement with @joen that these aren't strictly necessary here, especially if we create guidelines that encourage developers to avoid snackbars for critical (error) messages. Here's a quick idea of how this could potentially look: Let's look at how we can make this component as accessible as possible from the outset. Providing clear and consistent messaging in flexible ways has the potential to improve the product for all, by providing more helpful and contextual information without overloading users. I've done some looking around to establish guidelines, but it would be great to get the accessibility team's input here.
More reading: |
|
||
1. [Design guidelines](#design-guidelines) | ||
2. [Development guidelines](#development-guidelines) | ||
3. [Related components](#related-components) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a good idea to start adding "Accessibility guidelines" to all these component READMEs similar to dev and design. cc @audrasjb @LukePettway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great idea!
I updated the design, the a11y messages, I added a small README that can be iterated on, I didn't test everything yet but we can open this for review now. |
|
||
@include break-large() { | ||
width: calc(100% - #{ $admin-sidebar-width }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideall,y the styles that are specific to "WP Admin" should move to this stylesheet packages/edit-post/src/components/layout/style.scss
since the styles in this file can be used in any context (think a page where there's not sidebar...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 That's really helpful to know—I was wondering why there were styles for this component outside of the component itself. It makes things a bit confusing to work with. I'll try to move anything that refers to the sidebar over to that file.
@@ -30,7 +30,8 @@ | |||
@include editor-left(".edit-post-layout__content .components-editor-notices__snackbar"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this mixin is not needed anymore if you want to center them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Definitely had no clue what this was for. 😜
I've done my best to fix the design issues outlined my comment above with the exception of the animating-in issue, which we can look into in a separate PR. I had a bit of troubles with getting the notices centred on all permutations of the sidebar view. I think it may be that I'm not fully grokking how the For what it's worth, the second scenario here suffers the same issues with the left-aligned approach, so it's probably only critical that we fix the other two scenarios. I'd love some guidance here from someone who's a bit more familiar with the CSS code than I am, since I'm finding it a bit of a challenge to wade through. 🤯 |
That's exactly why I suggested that we keep the left aligned ones. I also prefer them visually (but I can see how this is just a personal difference). Maybe @jasmussen could help here |
Yes, this is quite messy. We have some mixins to take some of the vinegar out of here, but even then it's not superb. In fact although it may look nice when centered according to the main column, perhaps it would be worth to think of these notices outside of the boundaries of the editing canvas, but even think of them more as "global notices" for all of WordPress. Which means we could simply center them absolutely, regardless of left or right sidebars (easiest), or even left align them on desktop breakpoints like Gmail does: The material guidelines seem to suggest the same, which we can choose to take inspiration from. |
Would this close #15132 ? |
In an attempt to push this into a mergeable state and given the technical complexities and the fact that the left aligned snackbars can work just as well. I restored the left alignment for now. What's left in this PR? Can I get an approval? |
From a design POV, 👍 👍. We can always explore additional tweaks to how to position this best in any subsequent PRs, but it shouldn't block a wonderful component from shipping. All this needs is a code sanity check. |
🎉 Excellent work! Thanks for exploring this @youknowriad! |
Just want to add my two cents, that from a design perspective the snackbar notice feels much more elegant and simple. Love it! |
They are lovely. Admin has to either re-save to see the notice again or refresh the page to see the View post link in admin bar. |
This is a good point. In absence of a different interface for viewing post, perhaps that notification should be a dismissible notice instead of a snackbar. Would you mind opening a new ticket for this? Feel free to CC me. |
I think I disagree with this, when you publish a post, the "view link" is always visible on the post-publish panel unless you start typing. If we want to improve further, I'd rather think of ways to show an always visible "View post" link in the UI instead of relying on notices. |
One of our design principles for the snackbar notifications is that any action link will always be available elsewhere in the UI. This link isn't super prominent right now though, and (I suspect) that the most common action users want to take after publishing a post is viewing that post. IReverting back to a more disruptive notice may not be the best solution here—instead, we should either provide a more prominent link in the post-publish interface or consider redirecting the user to the post, and showing a notification with more post-publish options. (See: changes suggested in #7602.) |
closes #13905, closes #11807, closes #12127, closes #15132
This PR is a first stab towards using snackbars for post save/error and reusable blocks save/error notices.
Todo list:
You can test quickly by creating random notices in the console: