Skip to content
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

feat(ia): audience configuration boilerplate #3500

Merged
merged 36 commits into from
Nov 20, 2024
Merged

Conversation

jaredrethman
Copy link
Collaborator

@jaredrethman jaredrethman commented Oct 25, 2024

All Submissions:

Changes proposed in this Pull Request:

Adds Audience > Configuration Wizard. Existing React components were migrated to new directory (for the most part) as-is, the only exception is in the way Audience related wizards are presented using dynamic imports.

Adds Audience > Campaign boilerplate. This was done for 3 reasons:

  1. To provide an example of implementing further Audience related wizards.
  2. Supply a destination for call to actions within Audience > Configuration.
  3. If a admin menu (i.e. Audience) only has one submenu, no submenus are shown.

New menu & submenus:

Screenshot 2024-11-06 at 12 26 13

RAS:

Inactive Active
Screenshot 2024-11-07 at 09 23 53 Screenshot 2024-11-07 at 09 35 33

Important Information:

When comparing to Figma, there are a number of difference, mainly around missing settings namely; Platform, Salesforce and Stripe components - these will be added in a subsequent PR. When testing "Reader Revenue" prerequisite its important to remember that this UX will change, in that, instead of clicking through to a page it will open a Modal with all related settings i.e. Platform etc.

How to test the changes in this Pull Request:

  1. Checkout this branch and build assets git checkout origin/feat/ia-audience && npm run build
  2. It's best to test with as little of the Reader activation prerequisites active as possible, run the following CLI command to reset most Reader Activation prerequisites (depending on what you have configured locally you should see something resembling RAS > "Inactive" (above)):
wp option delete newspack_reader_activation_active_campaign_master_list newspack_reader_activation_contact_email_address newspack_reader_activation_enabled newspack_reader_activation_mailchimp_audience_id newspack_reader_activation_mailchimp_reader_default_status newspack_reader_activation_newsletter_lists newspack_reader_activation_newsletters_label newspack_reader_activation_sender_email_address newspack_reader_activation_sender_name newspack_reader_activation_sync_esp newspack_reader_activation_terms_text newspack_reader_activation_terms_url newspack_reader_activation_use_custom_lists _newspack_ras_skip_campaign_setup
  1. Step through and active each prerequisite. Ensure call-to-actions and back links navigate to correct locations and data persists.
  2. Once all prerequisites are active go ahead and test settings underneath "Show Advanced Settings".

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?

@jaredrethman jaredrethman changed the title Feat/ia audience feat(ia): audience boilerplate Oct 25, 2024
@@ -130,6 +130,16 @@ h1 {
/**
* Wizards
*/
// Only apply styles if there are sections and is immediate descendent.
.newspack-wizard__content:has(> .newspack-wizard__sections) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only apply styling to wizards wrapping components in WizardsTab component. See example

@jaredrethman jaredrethman changed the title feat(ia): audience boilerplate feat(ia): audience configuration boilerplate Nov 7, 2024
@jaredrethman jaredrethman marked this pull request as ready for review November 7, 2024 16:19
@jaredrethman jaredrethman requested a review from a team as a code owner November 7, 2024 16:19
@jaredrethman jaredrethman added the [Status] Needs Review The issue or pull request needs to be reviewed label Nov 7, 2024
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! All RAS configuration steps are working as expected.

When finishing the configuration, the prompts configuration didn't trigger the activation of RAS. It just redirected back to the steps with the "Reader Activation Campaign" as pending:

Gravacao.de.Tela.2024-11-14.as.11.00.45.mov

The Audience parent menu was mislocated, I pushed f433027 to fix the order:

image

When editing audience-related content (transactional emails and membership gate), we want "Audience -> Configuration" as the current menu. I pushed 41af659 to fix that:

Membership Gate Email
image image

On another note, do you mind deleting the duplicated files source? We don't need the previous wizards working. This will help with the cleanup and a more comprehensive diff.

@ronchambers
Copy link
Collaborator

ronchambers commented Nov 14, 2024

FYI: I merged epic/ia (to get the updated menu priority fixes), then I updated the parent menu vars in this commit: d87d0ab

Parent menu order:

image

@miguelpeixe
Copy link
Member

On another note, do you mind deleting the duplicated files source? We don't need the previous wizards working. This will help with the cleanup and a more comprehensive diff.

Pushed 4ea5532 and 2740cf7 to remove duplicate code

@miguelpeixe
Copy link
Member

miguelpeixe commented Nov 14, 2024

When finishing the configuration, the prompts configuration didn't trigger the activation of RAS. It just redirected back to the steps with the "Reader Activation Campaign" as pending:

This was caused by a small issue with the "Continue" button href. The newspackAudienceConfiguration.reader_activation_url has a trailing slash and ${ reader_activation_url }/complete generates [...]?page=newspack-audience-configuration#//complete, which doesn't work. Fixed in f572a65

@miguelpeixe
Copy link
Member

This looks good to me. Since I pushed a few changes, @ronchambers, do you mind reviewing this?

@ronchambers
Copy link
Collaborator

@miguelpeixe @jaredrethman - I tested locally and everything seems to work as expected. My react skills aren't good enough to do an approval, and I don't know Audience/RAS well enough to know what edge cases might be, but I did get though the initial configuration and then the RAS campaign setup too without any issues. Looks good! 😃

@jaredrethman
Copy link
Collaborator Author

@miguelpeixe @ronchambers thanks for the updates and reviews to this PR. I've gone through all the updates and am happy with the progress/changes that have been made. Can we go ahead and approve+merge?

Copy link
Collaborator

@ronchambers ronchambers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since @miguelpeixe said it looks good and Ron tested too, approved. @jaredrethman ok to merge.

@github-actions 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 20, 2024
@jaredrethman jaredrethman merged commit ed1837a into epic/ia Nov 20, 2024
8 checks passed
@jaredrethman jaredrethman deleted the feat/ia-audience branch November 20, 2024 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants