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

Media Kit page handling #3358

Merged
merged 4 commits into from
Sep 20, 2024
Merged

Media Kit page handling #3358

merged 4 commits into from
Sep 20, 2024

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Aug 22, 2024

All Submissions:

Changes proposed in this Pull Request:

  1. Removes references to the publisher-media-kit plugin, which will now be included in newspack-ads plugin.
  2. Change the Media Kit page handling in the Ads Wizard -> Add Ons tab
image

The three states of the UI:

image

How to test the changes in this Pull Request:

  1. Switch newspack-ads to feat/expose-media-kit branch (feat(media-kit): expose method; add get_page_status method newspack-ads#908)
  2. Visit the Advertising Wizard -> Add Ons tab
  3. Observe the UI acts as described in the comment above.

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?

The functionality is now included in the newspack-ads plugin
@adekbadek adekbadek added [Status] Needs Review The issue or pull request needs to be reviewed [Status] Needs Design Review labels Sep 13, 2024
@adekbadek adekbadek marked this pull request as ready for review September 13, 2024 08:39
@adekbadek adekbadek requested a review from a team as a code owner September 13, 2024 08:39
@adekbadek adekbadek force-pushed the feat/remove-publisher-media-kit branch from 4da5da8 to db04b2f Compare September 13, 2024 08:58
@kevinzweerink
Copy link

So I would recommend we follow the pattern we established for Everlit here:
https://www.figma.com/design/dvq52oOtkKpwYfKKFnGhUr/Newspack-Plugin?node-id=25960-7859&t=kFTbDOH1BoK2LoD6-1

What you've got is actually pretty close, I would just make a few small tweaks to get it more aligned:

  • The un-activated state stays the same as you've implemented
  • The intermediate (pending) state keeps the toggle in the off position. The emoji icon should be removed, and I would adjust the copy to say "Media kit page is created but unpublished, click the link to review and publish." And then the link text could be "Review draft page"
  • The active state can stay the same (except removing the emoji)

What state is shown if you toggle the card back off once it's fully activated? My recommendation would be that it go back to the pending state. Using the "off" position for the toggle makes more sense that way — otherwise you're toggling between "active" and "disabled" and the toggle can never truly be toggled off.

@adekbadek adekbadek removed the [Status] Needs Review The issue or pull request needs to be reviewed label Sep 19, 2024
@adekbadek
Copy link
Member Author

@kevinzweerink – thanks! I've update the code & PR description.

@adekbadek adekbadek added [Status] Needs Review The issue or pull request needs to be reviewed and removed [Status] Needs Design Review labels Sep 19, 2024
@adekbadek
Copy link
Member Author

@Automattic/newspack-product – this is ready for review now.

@adekbadek adekbadek changed the title feat(plugins): remove support for publisher-media-kit plugin Media Kit page handling Sep 19, 2024
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Not working as expected for me:

I've deleted the page, and the toggle is disabled and the subtitle is correct, but the link "review draft page" is there with a broken link.

image

When I click the toggle, the subtitle changes, but the page is not created. I see this in the logs:

PHP Warning:  Undefined array key "query" in /newspack-repos/newspack-plugin/includes/configuration_managers/class-newspack-ads-configuration-manager.php on line 212

@adekbadek
Copy link
Member Author

@leogermani – this would happen if you did not switch newspack-ads to the corresponding branch (feat/expose-media-kit). Nevertheless, I've added some error handling for this case.

@leogermani
Copy link
Contributor

this would happen if you did not switch newspack-ads to the corresponding branch (feat/expose-media-kit). Nevertheless, I've added some error handling for this case.

I was!

And just confirmed that if I checkout a70cf52 I still see the error I described.

But now in the latest commit of this branch it works as expected

@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 Sep 20, 2024
@leogermani leogermani merged commit 4454850 into trunk Sep 20, 2024
9 checks passed
@leogermani leogermani deleted the feat/remove-publisher-media-kit branch September 20, 2024 13:43
matticbot pushed a commit that referenced this pull request Sep 20, 2024
# [5.4.0-alpha.6](v5.4.0-alpha.5...v5.4.0-alpha.6) (2024-09-20)

### Bug Fixes

* change the current product criteria for sync ([#3416](#3416)) ([28a84bc](28a84bc))
* **esp-sync:** sync Connected Account field ([#3414](#3414)) ([61c02bc](61c02bc))
* hide My Account links if not relevant ([#3394](#3394)) ([0d039d8](0d039d8))
* **phpcs:** specify path in custom ruleset ref ([#3384](#3384)) ([b143e74](b143e74))
* prevent PHP notice while checking my-account page ([#3435](#3435)) ([146a26f](146a26f))
* **ras-sync:** deprecate redundant Signup_Page meta field ([#3439](#3439)) ([61d6de8](61d6de8))
* **reader-registration-block:** fix initial newsletter checkbox state ([1890efe](1890efe))
* replace `newspack_image_credits_placeholder` default value ([#3433](#3433)) ([c754fc2](c754fc2))
* **sync:** method name for `membership_saved` handler ([#3399](#3399)) ([2c0bf26](2c0bf26))
* **sync:** place esp sync admin features behind a constant ([#3438](#3438)) ([20a0970](20a0970))
* **sync:** remove localized number format ([#3434](#3434)) ([2243a5d](2243a5d))

### Features

* add a new action to when a ras setting is updated ([#3357](#3357)) ([35d3492](35d3492))
* **ga4:** detect gate interaction blocks ([#3408](#3408)) ([e14913c](e14913c))
* media kit page handling ([#3358](#3358)) ([4454850](4454850))
* **ras:** sync class ([#3362](#3362)) ([88acbee](88acbee))
* **ras:** unify ESP connector strategy for data events ([#3360](#3360)) ([7080864](7080864))
* **reader-activation:** ESP-related tweaks ([#3381](#3381)) ([ac68b67](ac68b67))
* remove Woo Membersip sync fields ([#3411](#3411)) ([28052e8](28052e8))
* **sync:** add ESP sync notice to RAS wizard ([#3400](#3400)) ([f9acd56](f9acd56))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 5.4.0-alpha.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Oct 3, 2024
# [5.5.0-alpha.1](v5.4.0...v5.5.0-alpha.1) (2024-10-03)

### Bug Fixes

* change the current product criteria for sync ([#3416](#3416)) ([28a84bc](28a84bc))
* **esp-meta:** handle state of the 'Woo Team' meta ([#3352](#3352)) ([ba5ea1e](ba5ea1e))
* **esp-sync:** sync Connected Account field ([#3414](#3414)) ([61c02bc](61c02bc))
* **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([d1abbfe](d1abbfe))
* **guest-author:** enqueue the guest author admin script selectively ([0bf37af](0bf37af))
* hide My Account links if not relevant ([#3394](#3394)) ([0d039d8](0d039d8))
* make email template fetching deterministic ([#3341](#3341)) ([ace91aa](ace91aa))
* **phpcs:** specify path in custom ruleset ref ([#3384](#3384)) ([b143e74](b143e74))
* prevent PHP notice while checking my-account page ([#3435](#3435)) ([146a26f](146a26f))
* **ras-sync:** deprecate redundant Signup_Page meta field ([#3439](#3439)) ([61d6de8](61d6de8))
* **reader-registration-block:** fix initial newsletter checkbox state ([1890efe](1890efe))
* replace `newspack_image_credits_placeholder` default value ([#3433](#3433)) ([c754fc2](c754fc2))
* **sync:** method name for `membership_saved` handler ([#3399](#3399)) ([2c0bf26](2c0bf26))
* **sync:** place esp sync admin features behind a constant ([#3438](#3438)) ([20a0970](20a0970))
* **sync:** remove localized number format ([#3434](#3434)) ([2243a5d](2243a5d))
* wizards - update type check conditional for `custom_logo` ([#3442](#3442)) ([125f756](125f756))
* woocommerce connection tests ([#3372](#3372)) ([5cea128](5cea128))
* **woocommerce-connection:** handle explicit UTM meta fields meta ([#3371](#3371)) ([bf9e997](bf9e997))

### Features

* add a new action to when a ras setting is updated ([#3357](#3357)) ([35d3492](35d3492))
* **esp-sync:** sync membership data regardless of subscription ([#3353](#3353)) ([9f7d1de](9f7d1de))
* **ga4:** detect gate interaction blocks ([#3408](#3408)) ([e14913c](e14913c))
* **ga:** disable tracking for editors regardless of RA status ([81323c3](81323c3))
* media kit page handling ([#3358](#3358)) ([4454850](4454850))
* **memberships:** add memberships-related body classes ([b56b9d8](b56b9d8))
* **ras:** esp sync tools ([#3359](#3359)) ([d7dd754](d7dd754))
* **ras:** helper method for ESP master list ([#3355](#3355)) ([ec56d5b](ec56d5b))
* **ras:** sync class ([#3362](#3362)) ([88acbee](88acbee))
* **ras:** unify ESP connector strategy for data events ([#3360](#3360)) ([7080864](7080864))
* **reader-activation:** ESP-related tweaks ([#3381](#3381)) ([ac68b67](ac68b67))
* remove Woo Membersip sync fields ([#3411](#3411)) ([28052e8](28052e8))
* **sync:** add ESP sync notice to RAS wizard ([#3400](#3400)) ([f9acd56](f9acd56))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 5.5.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Oct 8, 2024
# [5.5.0](v5.4.1...v5.5.0) (2024-10-08)

### Bug Fixes

* cancelled subscriptions sync ([#3466](#3466)) ([b605a7f](b605a7f))
* change the current product criteria for sync ([#3416](#3416)) ([28a84bc](28a84bc))
* **esp-meta:** handle state of the 'Woo Team' meta ([#3352](#3352)) ([ba5ea1e](ba5ea1e))
* **esp-sync:** sync Connected Account field ([#3414](#3414)) ([61c02bc](61c02bc))
* **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([d1abbfe](d1abbfe))
* **guest-author:** enqueue the guest author admin script selectively ([0bf37af](0bf37af))
* hide My Account links if not relevant ([#3394](#3394)) ([0d039d8](0d039d8))
* make email template fetching deterministic ([#3341](#3341)) ([ace91aa](ace91aa))
* **phpcs:** specify path in custom ruleset ref ([#3384](#3384)) ([b143e74](b143e74))
* prevent PHP notice while checking my-account page ([#3435](#3435)) ([146a26f](146a26f))
* **ras-sync:** deprecate redundant Signup_Page meta field ([#3439](#3439)) ([61d6de8](61d6de8))
* **reader-registration-block:** fix initial newsletter checkbox state ([1890efe](1890efe))
* replace `newspack_image_credits_placeholder` default value ([#3433](#3433)) ([c754fc2](c754fc2))
* **sync:** method name for `membership_saved` handler ([#3399](#3399)) ([2c0bf26](2c0bf26))
* **sync:** place esp sync admin features behind a constant ([#3438](#3438)) ([20a0970](20a0970))
* **sync:** remove localized number format ([#3434](#3434)) ([2243a5d](2243a5d))
* wizards - update type check conditional for `custom_logo` ([#3442](#3442)) ([125f756](125f756))
* woocommerce connection tests ([#3372](#3372)) ([5cea128](5cea128))
* **woocommerce-connection:** handle explicit UTM meta fields meta ([#3371](#3371)) ([bf9e997](bf9e997))

### Features

* add a new action to when a ras setting is updated ([#3357](#3357)) ([35d3492](35d3492))
* **esp-sync:** sync membership data regardless of subscription ([#3353](#3353)) ([9f7d1de](9f7d1de))
* **ga4:** detect gate interaction blocks ([#3408](#3408)) ([e14913c](e14913c))
* **ga:** disable tracking for editors regardless of RA status ([81323c3](81323c3))
* media kit page handling ([#3358](#3358)) ([4454850](4454850))
* **memberships:** add memberships-related body classes ([b56b9d8](b56b9d8))
* **ras:** esp sync tools ([#3359](#3359)) ([d7dd754](d7dd754))
* **ras:** helper method for ESP master list ([#3355](#3355)) ([ec56d5b](ec56d5b))
* **ras:** sync class ([#3362](#3362)) ([88acbee](88acbee))
* **ras:** unify ESP connector strategy for data events ([#3360](#3360)) ([7080864](7080864))
* **reader-activation:** ESP-related tweaks ([#3381](#3381)) ([ac68b67](ac68b67))
* remove Woo Membersip sync fields ([#3411](#3411)) ([28052e8](28052e8))
* **sync:** add ESP sync notice to RAS wizard ([#3400](#3400)) ([f9acd56](f9acd56))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @alpha [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.

4 participants