-
Notifications
You must be signed in to change notification settings - Fork 51
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
Conversation
@@ -130,6 +130,16 @@ h1 { | |||
/** | |||
* Wizards | |||
*/ | |||
// Only apply styles if there are sections and is immediate descendent. | |||
.newspack-wizard__content:has(> .newspack-wizard__sections) { |
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.
Only apply styling to wizards wrapping components in WizardsTab
component. See example
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 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:

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 | |
---|---|
![]() |
![]() |
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.
FYI: I merged Parent menu order: |
This was caused by a small issue with the "Continue" button href. The |
This looks good to me. Since I pushed a few changes, @ronchambers, do you mind reviewing this? |
@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! 😃 |
@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? |
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.
Since @miguelpeixe said it looks good and Ron tested too, approved. @jaredrethman ok to merge.
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:
New menu & submenus:
RAS:
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:
git checkout origin/feat/ia-audience && npm run build
Other information: