Feature/notification Design implementation (follow-up of #66)

[erikyo]

This is a followup of #66 PR

Following the @jonathanbossenger directions I've merged the #66 html templates into an installable plugin that shows the current design so that anyone installing it has a preview of the design.

Please however consider it as a WIP since some sections actually are merely sketched (for example the sidebar part is fully hardcoded). In addition there are some fix needed that @bacoords has pointed out

How to test:

• clone this branch, cd the plugin directory and wp-env start (you must have docker running)
• if the plugin works you should see at the top of the dashboard two buttons and a welcome notification
• Open devtools then into the console try with:
  addNotify({title: 'My title', message:'my message', image:'https://source.unsplash.com/random/400×400'}) or
  removeNotify(0) or clearNotifies()
• Check for Settings -> Notification (options-general.php?page=wp-notify)

[bacoords]

@erikyo One issue I noticed- admin notices from other plugins appear inside the initial admin notice instead of above/below.

[erikyo]

@erikyo One issue I noticed- admin notices from other plugins appear inside the initial admin notice instead of above/below.

@bacoords this looks like something related to the WordPress admin area js / jquery script that moves notifications below the first h2 of the page. If you try to disable wp-notify and reload the dashboard you will see that the yoast notification is created above the title and, after the page is loaded, it is moved below. I honestly think there is little we can do about that ¯_(ツ)_/¯

To fix this we should not use h2 headlines for the notification title... or someone has better ideas?

[folletto]

To fix this we should not use h2 headlines for the notification title... or someone has better ideas?

What about temporarily (until this plugin gets finalized) injecting a different empty H2 header that "captures" that JS movement?

[erikyo]

The script responsible is that movement is this:

$headerEnd = $( '.wp-header-end' );
if ( ! $headerEnd.length ) {
$headerEnd = $( '.wrap h1, .wrap h2' ).first();
}
$( 'div.updated, div.error, div.notice' ).not( '.inline, .below-h2' ).insertAfter( $headerEnd );

https://github.com/WordPress/WordPress/blob/master/wp-admin/js/common.js#L1083

But is easy to fix, we need to add a div with the class "wp-header-end" right below to wp-notify-dashboard-notices

<div id="wp-notify-dashboard-notices" class="wrap"></div>
<div class="wp-header-end"></div>

Could this be a good solution?

[Sephsekla]

The script responsible is that movement is this:

$headerEnd = $( '.wp-header-end' );
if ( ! $headerEnd.length ) {
$headerEnd = $( '.wrap h1, .wrap h2' ).first();
}
$( 'div.updated, div.error, div.notice' ).not( '.inline, .below-h2' ).insertAfter( $headerEnd );

Oh I long for the day when we can strip out jQuery from WP Core 😄

[jonathanbossenger]

Folks, I want to remind everyone that this code is purely to "demo" the notification design inside the admin dashboard. Hopefully fixing this doesn't take us down too deep of a rabbit hole, as it will probably be discarded when the actual implementation is worked on.

If we have to put it out there with a warning that it might conflict with existing admin_notices, I'm ok with that too. It's a demo, nothing more.

[bacoords]

I think a simple solution is to use <h3> elements instead of <h2> for the notice titles. Especially since we have an H2 for screen readers that says 'notifications', it makes sense that the notifications themselves each start with an H3.

[bacoords]

Just notes on the recent commits -

• @erikyo fixed the issue with notifications from other plugins (i.e. WooCommerce, Yoast) being pinned inside the demo
• I cleaned up the initial example notification so that it loads with the rest of the example notifications
• I removed the feature where you can test adding a notification by using the Quick Post dashboard widget. Instead I added a new dashboard widget that lets you add a demo notification so the Quick Post widget remains functional.

[jonathanbossenger]

Updating on naming. The reason this plugin "conflicts" with the other plugin in the repo is that the plugin folder is the same. So if everyone is ok with it, I'm going to rename this GitHub project to wp-feature-notifications.

[Sephsekla]

So if everyone is ok with it, I'm going to rename this GitHub project to wp-feature-notifications.

In my experience, github's smart enough to redirect any requests if you rename it, so it shouldn't cause any issues even if someone tries to push/pull from the old url.

[Sephsekla]

Can we get this branch merged into something on the main repo? Not necessarily the main develop branch, but it just adds an extra level of complexity that everything we're currently working on is actually in a fork.

[jonathanbossenger]

Folks, I took a look at the work done on the design_implementation branch. Well done, I like the way it loads in the dashboard and directs the user to try out the bell icon.

However, I do have a few concerns, that we need to discuss.

1. I'm guessing that the main dashboard notification is bound to some timer or event listener, which is why it doesn't load when the dashboard does. I understand why this is the case, but I worry that when we put this out for feedback, that's something folks are going to assume is part of the implementation.
2. I liked how our original demo showed all the different on-page notification variants, as well as all the different notification hub variants. While it was not interactive, it did give folks the ability to see all the different on-page and notification hub variants, whereas now I only see one variant for both.
3. I worry that if we spend more time going down this path, we spend more time developing something we might not eventually use. PR Feature/notification reworked design #66 was much closer to what we wanted to achieve in terms of showing how the different variants will look in the WordPress dashboard, in order to spark interest and gather feedback.

[erikyo]

I'm guessing that the main dashboard notification is bound to some timer or event listener, which is why it doesn't load when the dashboard does. I understand why this is the case, but I worry that when we put this out for feedback, that's something folks are going to assume is part of the implementation.

This is something I was working on and I hadn't made it explicit yet, related to animations. The first notification you see in the dash has a 2000ms delay, this is because during development I need to verify that the controller keeps updating. From my point of view I think it is awesome if the notifications are in real time and not like now when you load the page (as now in WordPress) so I'm taking care that it can be done. Nothing precludes the fact that they can be loaded immediately.

I liked how our original demo showed all the different on-page notification variants, as well as all the different notification hub variants. While it was not interactive, it did give folks the ability to see all the different on-page and notification hub variants, whereas now I only see one variant for both.

You are right but I would point out that this is still a WIP, I recently created the notification container class component exactly because it will be used there as well. Anyway, in the next commit I will add some fake notifications again to make it the same as before, because now it is indeed a bit empty.

I worry that if we spend more time going down this path, we spend more time developing something we might not eventually use. PR #66 was much closer to what we wanted to achieve in terms of showing how the different variants will look in the WordPress dashboard, in order to spark interest and gather feedback.

Project #66 was interesting yes but also very limited and could show a first draft of what the plugin could look like (which it keeps doing here).... However to move on to a next step mounting it inside WordPress was necessary. In this way it can be tested in the right way inside the wp environment, facing yet the first issues like the jquery notification position hijacker.

[bacoords]

@Sephsekla I believe any of us contributing on this fork (@erikyo and me so far) would need to be invited to the main repo to have permissions to work on the PR as a branch here, correct?

@jonathanbossenger It seems there's a concern about whether we're loading notifications before page load via PHP or after via JS. My default assumption would be that we're moving towards doing everything after load with JS, for a few reasons:

• the better to interact with the block editor, especially if we're going to eventually deal with the fact that the block editor mostly hides the top admin bar...
• The current admin notices 'jump' with jQuery to their proper place on the page anyway, so this is intentionally smoother
• There's a fake delay in this demo code that can simply be removed for now.
• The project guidelines seem to suggest moving away from using a PHP hook to load a notice and instead throwing it into the database and then fetching them with a REST API endpoint?

So, main questions:

• Should we stick to loading with JS and just remove the delay for this version?
• Do we want the sidebar 'mark as read' to be functional for the demo or not?
• Are we concerned about mobile at this point?
• Does anyone else think that clicking outside of the drawer-style sidebar should make it disappear? That's a pretty common pattern and I don't see any other UI element to close the sidebar.

[Sephsekla]

Does anyone else think that clicking outside of the drawer-style sidebar should make it disappear? That's a pretty common pattern and I don't see any other UI element to close the sidebar.

Personally as a user, that's definitely the behaviour I'd expect it to have.

[erikyo]

Do we want the sidebar 'mark as read' to be functional for the demo or not

To do that you would have to create another "Notifications" in the sidebar, it is in the plan but then the operation will be the same as the dashboard (with the clear all button)

Are we concerned about mobile at this point?

Maybe but when I did it they were not specified and I didn't want to invent anything.... this might answer you better @folletto

Does anyone else think that clicking outside of the drawer-style sidebar should make it disappear? That's a pretty common pattern and I don't see any other UI element to close the sidebar.

This is definitely a good idea! I'm adding this one to the todo!

[folletto]

Are we concerned about mobile at this point?

I'd say that's not a priority for the demo, but we also want to avoid to be in a situation where everything needs to be rewritten because we didn't consider mobile from the start. As long as the foundation is good, I think it's ok not to immediately prioritize it and build it up as we validate the functionality :)