Reintroduce JITMs in the WC Admin. #34383

AnnaMag · 2022-08-18T21:26:13Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Enable the JITMs in the Admin sections.

The JITMs haven't been added to the Onboarding wizard in order to keep the same experience. We can add them later if we want.

Closes # 60-gh-woocommerce/mothra-private

How to test the changes in this Pull Request:

Install WC on a connected Jetpack site.
To be able to see a testing JITM, you'll need to add &test_jitm=/1/ at the end of a WooCommerce URL. It will work on every WooCommerce page except for: Products > Categories, Products > Tags, Products > Attributes, and Products > Reviews.
Verify that the JITMs look good using the new navigation.
They should look good also with different browser widths.
Verify that they are not being shown in the Onboarding wizard.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?
Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

github-actions · 2022-08-18T21:46:30Z

Test Results Summary

Commit SHA: d403c76

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	118	0	0	2	0	120	1m 1s
E2E Tests	186	0	0	1	0	187	12m 26s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

beaulebens · 2022-08-31T18:22:20Z

plugins/woocommerce/src/Internal/Admin/Loader.php

-
-		// Wrap the notices in a hidden div to prevent flickering before
-		// they are moved elsewhere in the page by WordPress Core.
-		echo '<div class="woocommerce-layout__notice-list-hide" id="wp__notice-list">';


I think (requires testing) if we remove this, then we will probably completely disable all of the admin-notices-cleanup work that WC Admin does (e.g. any notices injected by any plugin will come back on all screens). JITMs do also support being inserted into a different container (<div id="jp-admin-notices" />) so maybe we'd be better off keeping the existing cleanup, and then carefully placing that container?

Hi @beaulebens, thank you for your comment,

I've been working on this PR. Since the class woocommerce-layout__notice-list-hide essentially only adds a display none (so it's not being shown), I removed the class and replaced the div id with jp-admin-notices, otherwise, we would add the JITMs twice and hide one of them.

I couldn't find a scenario where that change breaks the experience, but if we can find one I'll leave that div as it is now and add a new one with the id jp-admin-notices.

Thanks for adding this back in, this is testing very well now, I am only seeing the Jetpack notices.

louwie17

This tested well for me and code looks good!
I will approve this after we get a consensus on this comment: https://github.com/woocommerce/woocommerce/pull/34383/files#r959890093

As these changes will show all the notices again, like this Paypal payments for example:

Users might indeed get a pleasant surprise, as I noticed that one of my older sites had several notices that would otherwise be hidden. But shown on WP > Dashboard.
I wonder if we could wrap these notices in a nicer format that matches our designs better (cc @jarekmorawski, @pmcpinto ), but that would be out of scope if we wanted to get this in by Thursday (for the code freeze).
It might be nicer to have them part of some kind of dropdown on any WooCommerce screens so it's less intrusive.

beaulebens · 2022-09-12T16:22:47Z

As these changes will show all the notices again, like this Paypal payments for example

The preference here was definitely to show JITMs again, but not to suddenly start showing all notices (which should ideally still be collected up and tucked away, as before.

octaedro · 2022-09-12T18:33:39Z

Thank you @louwie17 and @beaulebens for your feedback,
I just addressed the changes you suggested.
Could you take another look at this PR?

louwie17

Nice work @octaedro, this seems to test well now, I don't see all the notices but only the JITM's.
I just left one small comment on the styling, but otherwise this is great!

louwie17 · 2022-09-12T19:57:06Z

plugins/woocommerce/src/Internal/Admin/Loader.php

-
-		// Wrap the notices in a hidden div to prevent flickering before
-		// they are moved elsewhere in the page by WordPress Core.
-		echo '<div class="woocommerce-layout__notice-list-hide" id="wp__notice-list">';


Thanks for adding this back in, this is testing very well now, I am only seeing the Jetpack notices.

louwie17 · 2022-09-12T20:08:37Z

plugins/woocommerce-admin/client/stylesheets/shared/_global.scss

+	&__jitm {
+		.jitm-card {
+			margin-left: 1.25rem !important;
+			margin-bottom: 2.5rem;


The margin-bottom 2.5rem feels like a lot on the older pages like products list, could we potentially also make this 1.25rem?

Timstreep · 2022-09-13T07:52:44Z

@beaulebens there's a few legacy JITMs hiding in the system that should be tested before this goes live. I've documented these here. They're pretty old, so there's no telling how they might look when reintroduced or if the links still work properly. I wouldn't be opposed to disabling these altogether and starting with a clean slate.

louwie17 · 2022-09-13T14:16:37Z

@beaulebens there's a few legacy JITMs hiding in the system that should be tested before this goes live. I've documented these here. They're pretty old, so there's no telling how they might look when reintroduced or if the links still work properly. I wouldn't be opposed to disabling these altogether and starting with a clean slate.

@Timstreep I just went through that list and managed to get all of them besides one to show up. The only one I was having trouble with was the product_addons one. Not sure if this is related to the required of at-least one order and it not having been synced yet.

louwie17

Thanks for sticking with the several styling updates @octaedro, this is testing well for me and looking great! Thanks so much for your hard work on this.
You got my approval and is good to merge from my perspective as long as all the checks pass :)

louwie17

Re-approving after rebase.

github-actions · 2022-09-14T16:06:13Z

Hi @octaedro, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

Add the release: add testing instructions label

jarekmorawski · 2022-09-15T07:42:03Z

Hi there! Can you tell me why we're bringing the notices back to wc-admin? Most merchants find them overwhelming, and with the Inbox and the task list in place, I am not sure why we would need another notification system.

What are we hoping to achieve with this? What's our long-term goal? Do we imagine this being limited to Jetpack or open to all extensions? cc @beaulebens

beaulebens · 2022-09-15T15:28:41Z

They are being brought back because they provide more contextual messaging, available on each specific screen. Note that the visuals you see here are just the basic/default theme since this was originally created by Jetpack; we will be using Woo messaging/designs, and this is not available to other plugins (100% controlled by us). We can and should definitely iterate on the design to make it something that fits into WC Admin better, but right now we wanted to at least re-enable it so that we could experiment with it in different contexts.

jarekmorawski · 2022-09-16T09:01:28Z

I understand the value of JITMs and would love to find a better way to show them in the system. However, as demonstrated by the recent Inbox note bug (p1663270180977999-slack-C2CM2ELTG), people do not react positively to additional messaging in the admin UI. Maybe we could start a high-priority project where we'd focus on the notification system in general and think of ways to balance user and business needs? I'm sure we could find a middle ground and build something that would serve our needs for years to come.

github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Aug 18, 2022

AnnaMag added this to the 7.0.0 milestone Aug 19, 2022

beaulebens reviewed Aug 31, 2022

View reviewed changes

octaedro force-pushed the add/jitms-in-wcadmin branch from 7e1dcdf to d57c8fa Compare September 9, 2022 19:53

octaedro requested a review from a team September 9, 2022 20:41

octaedro marked this pull request as ready for review September 9, 2022 20:41

octaedro self-assigned this Sep 9, 2022

louwie17 reviewed Sep 12, 2022

View reviewed changes

github-actions bot added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Sep 12, 2022

octaedro force-pushed the add/jitms-in-wcadmin branch from e5c2540 to d2fcc3b Compare September 12, 2022 18:09

github-actions bot removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Sep 12, 2022

louwie17 reviewed Sep 12, 2022

View reviewed changes

louwie17 previously approved these changes Sep 13, 2022

View reviewed changes

octaedro dismissed louwie17’s stale review via 5000841 September 13, 2022 18:00

octaedro and others added 9 commits September 13, 2022 17:13

Add JITMs

07af460

Fix styles

8a67f0f

Remove styles

22886e7

Add changelog.

947c8b4

Add notice-list div

e15a6d0

Move hidden jitm style

d491337

Move hidden jitm style

e1872d9

Fix style

0c0f3de

Fix margin-right

1c8803a

octaedro force-pushed the add/jitms-in-wcadmin branch from de99182 to 1c8803a Compare September 13, 2022 20:15

Remove important

d403c76

louwie17 approved these changes Sep 14, 2022

View reviewed changes

octaedro merged commit 01925e6 into trunk Sep 14, 2022

octaedro deleted the add/jitms-in-wcadmin branch September 14, 2022 16:05

Reintroduce JITMs in the WC Admin. #34383

Reintroduce JITMs in the WC Admin. #34383

Uh oh!

Conversation

AnnaMag commented Aug 18, 2022 • edited by octaedro Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

Uh oh!

github-actions bot commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

beaulebens Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

octaedro Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

louwie17 Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

louwie17 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beaulebens commented Sep 12, 2022

Uh oh!

octaedro commented Sep 12, 2022

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

louwie17 Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

louwie17 Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

Timstreep commented Sep 13, 2022

Uh oh!

louwie17 commented Sep 13, 2022

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

louwie17 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 14, 2022

Uh oh!

jarekmorawski commented Sep 15, 2022

Uh oh!

beaulebens commented Sep 15, 2022

Uh oh!

jarekmorawski commented Sep 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AnnaMag commented Aug 18, 2022 •

edited by octaedro

Loading

github-actions bot commented Aug 18, 2022 •

edited

Loading

louwie17 left a comment •

edited

Loading