Feat / IA for Newsletters #3490

ronchambers · 2024-10-21T18:23:00Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go 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:

Once the "TODOs" below are done, this PR will complete the Information Architecture Wizard for the Newsletters Plugin. This code will update the existing Newsletters Plugin header and menu.

See screenshot:

The Newsletters custom blue header is deactivated.
A new Admin Header is added.
The menu is moved and compressed.
Some menu items are now combined into Tabs.

Additionally the new Dashboards page is updated so that if the Newsletters plugin is not installed, then the dashboard will not show Newsletters items.

See screenshot:

TODOs:

We may need to change the menu icon. Export the SVG from Figma or talk to Thomas/designer. See code comment: // @TODO get SVG from Figma? This one is "envelope" from: https://wordpress.github.io/gutenberg/?path=/story/icons-icon--library
I was having a hard time setting the Newsletters CPT parent menu item to a decimal value. It seems that CPTs only support an integer value for menu_positon. So I created a function that will loop through global $menu and then move the CPT to position 3.3 or "3.33" if something is already at 3.3. See the current code in function move_cpt_menu(). You may want to look at function registered_post_type_newsletters( $post_type ) to see how I'm changing the menu_icon but when I try to change the menu_position to 3.3 in that function it doesn't work.
I didn't have chance to fully test what happens if someone goes to a "bookmarked" category or tag page that was removed by this Wizard. I recommended we remove categories and tags from the Newsletters menu in this PR ( Feat: Information Architecture for Newsletters newspack-newsletters#1678 ) but still, even with the links removed, someone could have bookmarked any of the following links, so we may want to view them in the browser and see what the user would see. We'll probably want to remove the "blue" Newsletters header and apply the new Admin Header like this code does for the other screens.
- edit-tags.php?taxonomy=category&post_type=newspack_nl_cpt
- edit-tags.php?taxonomy=post_tag&post_type=newspack_nl_cpt
- term.php?taxonomy=category&tag_ID=4&post_type=newspack_nl_cpt
- term.php?taxonomy=post_tag&tag_ID=31&post_type=newspack_nl_cpt
There is an existing Newsletters admin screen in the current (trunk) Newspack Plugin. It can be seen in wp-admin Newspack > Engagement > Newsletters or admin.php?page=newspack-engagement-wizard#/newsletters. It looks to be a React version of the existing Newsletters Settings screen. Typically I'm not a fan of having different screens that do the same thing, so ... maybe we can just remove the React version when IA goes live since Newsletters already has a Settings screen? But I'm not in charge 😉 If it is kept, I'm not sure how to "plug it in" so that admin users that have the Newspack Plugin with IA active get "redirected/shown" the React version of the screen vs admin users that just use Newsletters stand-alone without the Newspack Plugin who will just see the normal settings page.
Finally, I'm not sure why ci/cirlceci:test-php is failing in github below. Maybe it was an error that got merged into this branch via trunk or epic/ia?

How to test the changes in this Pull Request:

Pull branch.
npm run build
Activate Newspack plugin.
Install/activate Newspack Newsletters Plugin.
Verify screenshots and notes above.
Deactivate Newsletters plugin, verify dashboard no longer shows Newsletters section.
Re-activate the Newsleters plugin and do the one test related to get_settings_url() in the related PR ( Feat: Information Architecture for Newsletters newspack-newsletters#1678 )

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 ran tests with your changes locally?

ronchambers · 2024-10-25T04:46:34Z

@miguelpeixe - I assigned you in Asana. I'll reach out in slack.

miguelpeixe · 2024-10-29T19:28:34Z

Addressing the remaining work:

We may need to change the menu icon. Export the SVG from Figma or talk to Thomas/designer. See code comment: // @todo get SVG from Figma? This one is "envelope" from: https://wordpress.github.io/gutenberg/?path=/story/icons-icon--library

I've settled on using Gutenberg's envelope with proper styles to match different states:

Selected	Unselected

I was having a hard time setting the Newsletters CPT parent menu item to a decimal value. It seems that CPTs only support an integer value for menu_positon. So I created a function that will loop through global $menu and then move the CPT to position 3.3 or "3.33" if something is already at 3.3. See the current code in function move_cpt_menu(). You may want to look at function registered_post_type_newsletters( $post_type ) to see how I'm changing the menu_icon but when I try to change the menu_position to 3.3 in that function it doesn't work.

The menu order handling has been refactored in #3501 and merged to this PR.

I didn't have chance to fully test what happens if someone goes to a "bookmarked" category or tag page that was removed by this Wizard.

This seems like a very edge case, which I didn't tackle here. If this becomes a problem, we can setup redirects later on.

There is an existing Newsletters admin screen in the current (trunk) Newspack Plugin. It can be seen in wp-admin Newspack > Engagement > Newsletters or admin.php?page=newspack-engagement-wizard#/newsletters. It looks to be a React version of the existing Newsletters Settings screen.

This wizard should reflect the react version, under Engagement, not the one coming from the Newsletters plugin. The Newsletters version should remain unchanged and available in case the main plugin is not installed.

I've refactored this PR to move the wizard from the Engagement tab to the root level under the Newsletters menu. This also means moving the methods from includes/wizards/class-engagement-wizard.php to includes/wizards/class-newsletters-wizard.php.

To match the proposed tabs with "Tracking", a new wizard was created for those settings:

Settings	Tracking

We were also missing the wizard header when creating and editing local subscription lists:

Testing

Checkout this branch and, without the Newsletters plugin active, confirm the menu is not available
Activate Newsletters and confirm the menu is now available
Visit all links on the Newsletters menu and confirm the expected pages render
Click "Newsletters -> Settings", configure and ESP and hit Save
Refresh the page and confirm the changes persist
Toggle Subscription Lists and Save
Refresh the page and confirm the changes persist
Click "Add New" on Subscription Lists
Confirm the editor renders with the correct wizard header
Save the list and click "<< Back to Subscription Lists management"
Confirm you're back to the Settings wizard
Navigate to the "Tracking" tab
Toggle both options, refresh the page and confirm they persist

jaredrethman

@miguelpeixe this is great!

We were also missing the wizard header when creating and editing local subscription lists:

Nice catch - thanks for adding this.

Testing

Checkout this branch and, without the Newsletters plugin active, confirm the menu is not available

Activate Newsletters and confirm the menu is now available

Visit all links on the Newsletters menu and confirm the expected pages render

Click “Newsletters -> Settings”, configure and ESP and hit Save

Refresh the page and confirm the changes persist

Toggle Subscription Lists and Save

Refresh the page and confirm the changes persist

Click “Add New” on Subscription Lists

Confirm the editor renders with the correct wizard header

Save the list and click “<< Back to Subscription Lists management”

Confirm you’re back to the Settings wizard

Navigate to the “Tracking” tab

Toggle both options, refresh the page and confirm they persist

All tests work as described. Some feedback:

Submenu highlighting:
- Not highlighting /wp-admin/post-new.php?post_type=newspack_nl_cpt (template select and edit screen). Should highlight "All Newsletters"?
- Not highlighting /wp-admin/post-new.php?post_type=newspack_nl_ads_cpt. Should highlight "Advertising"?
- "All Newsletters" highlighted when on Add New Lists (/wp-admin/post-new.php?post_type=newspack_nl_list) - is this intentional?
- Is Newsletters > Tracking new (not in Figma)?
Newsletters > Settings
- No space between sections. This bug was introduced when refactoring general Wizard section(s) styling. This should be handled separate from this PR.
- WooCommerce integration missing?
Settings tabs missing headers. Have added suggestions below.

jaredrethman · 2024-10-31T14:05:44Z

src/wizards/newsletters/views/settings/index.js

 	const [ { newslettersConfig }, updateConfiguration ] = hooks.useObjectState( {} );
 	const [ initialProvider, setInitialProvider ] = useState( '' );
 	const [ lockedLists, setLockedLists ] = useState( false );
 	const [ authUrl, setAuthUrl ] = useState( false );

 	return (
 		<>
-			<NewspackNewsletters
+			<Settings


Suggested change

<Settings

<h1>{ __( 'Newsletter Settings', 'newspack-plugin' ) }</h1>

<Settings

It feels like we're saying the same thing 3 times:

It's how it's proposed in the design, but since the design screen doesn't have the wizard header, maybe it slipped by. @thomasguillot could you confirm that this is the intended result?

I think it's ok if we repeat things and it's to have some sort of consistency with the other wizards. We could probably the heading to simply "Settings"

Added in ac26dbc

Like this?

Yes

jaredrethman · 2024-10-31T14:06:19Z

src/wizards/newsletters/views/tracking/index.js

+
+	return (
+		<>
+			<ActionCard


Suggested change

<ActionCard

<h1>{ __( 'Newsletter Tracking', 'newspack-plugin' ) }</h1>

<ActionCard

Same as above:

Same here, I'd say "Tracking" is sufficient

Added in ac26dbc

miguelpeixe · 2024-10-31T17:59:03Z

Thansk for the review, @jaredrethman!

Not highlighting /wp-admin/post-new.php?post_type=newspack_nl_cpt (template select and edit screen). Should highlight "All Newsletters"?
Not highlighting /wp-admin/post-new.php?post_type=newspack_nl_ads_cpt. Should highlight "Advertising"?
"All Newsletters" highlighted when on Add New Lists (/wp-admin/post-new.php?post_type=newspack_nl_list) - is this intentional?

Nice catch! After some investigation I finally understood how submenu_file and parent_file works. Fixed in c4df9b0.

Unfortunately, as stated in the code, I was unable to make the Settings submenu get highlighted when editing a Subscription List. The $submenu_file is empty for edit.php?post_type=newspack_nl_cpt&page=newspack-newsletters and I can't figure out how to point to that item.

Is Newsletters > Tracking new (not in Figma)?

Yes. We missed it, but it's required.

This should be handled separate from this PR.

👍

WooCommerce integration missing?

Whoops! I missed it. Added in bb131a9

jaredrethman · 2024-11-01T14:22:55Z

@miguelpeixe thank you for your changes/fixes.

Unfortunately, as stated in the code, I was unable to make the Settings submenu get highlighted when editing a Subscription List. The $submenu_file is empty for edit.php?post_type=newspack_nl_cpt&page=newspack-newsletters and I can't figure out how to point to that item.

If highlighting "Settings" when adding/editing a Subscription List is what we want it would require updating:

newspack-plugin/includes/wizards/class-newsletters-wizard.php

Lines 507 to 513 in bb131a9

    
           // Move newsletter subscription list submenu_file. 
        
           if ( ! empty( $submenu_file ) && strpos( $submenu_file, 'newspack_nl_list' ) !== false ) { 
        
           	// This would ideally be under &page=newspack-newsletters to match the 
        
           	// Settings submenu, but it's not reachable so we go with the second best 
        
           	// possibility. 
        
           	return 'edit.php?post_type=newspack_nl_cpt'; 
        
           }

to:

if ( ! empty( $submenu_file ) && strpos( $submenu_file, 'newspack_nl_list' ) !== false ) {
	return $this->slug;
}

miguelpeixe · 2024-11-01T14:26:45Z

🤯 thank you!! Updated in f9442fb

miguelpeixe · 2024-11-01T14:38:01Z

@jaredrethman I've also just pushed b6a9f7e to address 1205919985867982-as-1208668902916641/f

There's no need to have the ESP settings ActionCard inside a div. There's no instance in which className is used. Not here and not in the onboarding wizard, which makes use of the same component.

jaredrethman

@miguelpeixe nice work here - appreciate the revisions! Approved!

ronchambers added 2 commits October 20, 2024 17:04

feat(ia-newsletters): init

6dd62df

feat(ia-newsletters): adjustments to menu for cpt. also dashboard.

694ab2d

ronchambers changed the base branch from trunk to epic/ia October 21, 2024 18:23

ronchambers added 6 commits October 21, 2024 15:40

feat(ia-newsletters): hooks for CPT and Taxonomy

2b7d902

feat(ia-newsletters): combine screens into tabs, submenu file, get slug

f6186d3

feat(ia-newsletters): bug need to fix

1c1113f

feat(ia-newsletters): fixed admin title bug

65cad2f

Merge branch 'epic/ia' into feat/ia-newsletters

432f8b9

feat(ia-newletters): allow force selected for admin tabs

9aa120c

ronchambers mentioned this pull request Oct 23, 2024

Feat: Information Architecture for Newsletters Automattic/newspack-newsletters#1678

Closed

6 tasks

ronchambers added 4 commits October 24, 2024 09:24

Merge branch 'epic/ia' into feat/ia-newsletters

355b942

feat(ia-newsletters): clean up

7a3a956

feat(ia-newsletters): clean up prior to final lint and test

253a35f

feat(ia-newsletters): linting

645ecb2

miguelpeixe added 2 commits October 25, 2024 11:26

fix: update plugin check

c2780b2

fix: svg

26a6106

miguelpeixe mentioned this pull request Oct 25, 2024

refactor: wizards menu order handling #3501

Merged

6 tasks

miguelpeixe added 7 commits October 25, 2024 15:36

refactor: wizards menu order handling (#3501)

435cf72

Merge remote-tracking branch 'origin/epic/ia' into feat/ia-newsletters

efb1d70

fix: move newsletter wizard js (wip)

de56d08

feat: move newsletters wizard

6e197ac

fix: remove css that disrupts gutenberg elements

4ac6e51

feat: tracking wizard

c5bdc44

fix: menu file

b5c30c4

miguelpeixe marked this pull request as ready for review October 29, 2024 19:28

miguelpeixe requested a review from a team as a code owner October 29, 2024 19:28

miguelpeixe requested a review from jaredrethman October 29, 2024 19:28

miguelpeixe self-assigned this Oct 29, 2024

miguelpeixe added the [Status] Needs Review The issue or pull request needs to be reviewed label Oct 29, 2024

jaredrethman requested changes Oct 31, 2024

View reviewed changes

miguelpeixe added 4 commits October 31, 2024 13:19

chore: add titles

ac26dbc

fix: move file for newsletters' post-new.php

c410fad

fix: parent and submenu files

c4df9b0

feat: add "WooCommerce Integration"

bb131a9

miguelpeixe requested a review from jaredrethman October 31, 2024 17:59

fix: submenu file for subscription lists

f9442fb

fix: remove unnecessary nodes

b6a9f7e

jaredrethman reviewed Nov 1, 2024

View reviewed changes

jaredrethman approved these changes Nov 1, 2024

View reviewed changes

github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Nov 1, 2024

miguelpeixe merged commit 8ad5986 into epic/ia Nov 1, 2024
9 checks passed

miguelpeixe deleted the feat/ia-newsletters branch November 1, 2024 15:02

miguelpeixe mentioned this pull request Nov 1, 2024

fix(ia): dashboard newsletters settings link #3515

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat / IA for Newsletters #3490

Feat / IA for Newsletters #3490

ronchambers commented Oct 21, 2024 •

edited

Loading

ronchambers commented Oct 25, 2024

miguelpeixe commented Oct 29, 2024

jaredrethman left a comment

jaredrethman Oct 31, 2024

miguelpeixe Oct 31, 2024

thomasguillot Oct 31, 2024

miguelpeixe Oct 31, 2024

miguelpeixe Oct 31, 2024

miguelpeixe Oct 31, 2024

thomasguillot Oct 31, 2024

jaredrethman Oct 31, 2024

miguelpeixe Oct 31, 2024

thomasguillot Oct 31, 2024

miguelpeixe Oct 31, 2024

miguelpeixe commented Oct 31, 2024

jaredrethman commented Nov 1, 2024

miguelpeixe commented Nov 1, 2024

miguelpeixe commented Nov 1, 2024

jaredrethman left a comment

	<Settings
	<h1>{ __( 'Newsletter Settings', 'newspack-plugin' ) }</h1>
	<Settings

	<ActionCard
	<h1>{ __( 'Newsletter Tracking', 'newspack-plugin' ) }</h1>
	<ActionCard

Feat / IA for Newsletters #3490

Feat / IA for Newsletters #3490

Conversation

ronchambers commented Oct 21, 2024 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

TODOs:

How to test the changes in this Pull Request:

Other information:

ronchambers commented Oct 25, 2024

miguelpeixe commented Oct 29, 2024

Testing

jaredrethman left a comment

Choose a reason for hiding this comment

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelpeixe commented Oct 31, 2024

jaredrethman commented Nov 1, 2024

miguelpeixe commented Nov 1, 2024

miguelpeixe commented Nov 1, 2024

jaredrethman left a comment

Choose a reason for hiding this comment

ronchambers commented Oct 21, 2024 •

edited

Loading