Skip to content

fix: restore Media Kit components in Ads wizard#3881

Merged
dkoo merged 4 commits into
releasefrom
fix/restore-media-kit-settings
Apr 3, 2025
Merged

fix: restore Media Kit components in Ads wizard#3881
dkoo merged 4 commits into
releasefrom
fix/restore-media-kit-settings

Conversation

@dkoo

@dkoo dkoo commented Apr 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

All Submissions:

Changes proposed in this Pull Request:

Hotfix to add some IA reorganization that was missed.

  • Restores the Media Kit settings component in the Advertising > Display Ads > Settings wizard page (behavior should remain as described in Media Kit page handling #3358)
  • Moves the Suppression settings component from its own view to the Advertising > Display Ads > Settings wizard page
  • Changes the visual style of the Suppression settings section to more closely match other settings components on the page (not in the design mockup)

Note: I did not move the Super Cool Ad Inserter plugin settings to the "Plugins" settings in this PR, as is outlined in the design mockup. These settings are more tightly integrated into the Newspack Ads plugin and moving them would require a more extensive refactor that doesn't fit the urgency of this issue. cc @thomasguillot if we'd like to flag this for a future refactor.

Screenshot 2025-04-02 at 17-20-53 Advertising ‹ dkoo — WordPress

How to test the changes in this Pull Request:

  1. Check out this branch
  2. In WP admin, visit Advertising > Display Ads > Settings
  3. Confirm that Suppression settings appear in this page, as well as Ad Refresh Control and Media Kit settings under a "Plugins" heading
  4. Confirm that settings in all sections can be updated and saved and that updates persist after reloading the page

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?

@dkoo dkoo added the [Status] Needs Review The issue or pull request needs to be reviewed label Apr 2, 2025
@dkoo dkoo self-assigned this Apr 2, 2025
@dkoo dkoo requested a review from a team as a code owner April 2, 2025 23:26
@thomasguillot

thomasguillot commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

We need to have a "Plugins" tab because why would the page has heading that says "Settings" and then we scroll down and there's another heading, this time that says "Plugins".

laurelfulford
laurelfulford approved these changes Apr 3, 2025
View reviewed changes

@laurelfulford laurelfulford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for tackling this, @dkoo! Everything saves as expected, and TIL Ad Refresh Control was a separate plugin?

The only thing I noticed was that the Suppression section isn't 1:1 with how it looks in the mockup (with the header, and without the border), but I think that could be tackled in a second, less urgent iteration.

I'm marking as approved from a functional perspective, but I'm happy to take another look if you end up breaking 'Plugins' into its own tab in this PR like @thomasguillot recommended.

CleanShot 2025-04-03 at 08 29 36

@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 Apr 3, 2025
@dkoo

dkoo commented Apr 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks, @laurelfulford! Yeah, re: the "Suppression" settings, I'm proposing here that we wrap the section in a card like the other settings sections so that the page looks more visually consistent. Ok with you, @thomasguillot?

Note that we've also decided to eliminate the "Plugins" heading halfway down the page. The whole page now looks like this. with all settings sections existing at the same hierarchy in the Settings page:

Screenshot 2025-04-03 at 11-15-39 Advertising ‹ dkoo — WordPress

I'll flag this as needing a design review from @thomasguillot since the end result is a bit different than the original designs.

@dkoo dkoo requested a review from thomasguillot April 3, 2025 17:14
thomasguillot
thomasguillot approved these changes Apr 3, 2025
View reviewed changes

@thomasguillot thomasguillot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@dkoo dkoo merged commit 86fa89a into release Apr 3, 2025
@dkoo dkoo deleted the fix/restore-media-kit-settings branch April 3, 2025 20:52
matticbot pushed a commit that referenced this pull request Apr 3, 2025
@semantic-release-bot
chore(release): 6.2.2 [skip ci]
1008d6e 
## [6.2.2](v6.2.1...v6.2.2) (2025-04-03)

### Bug Fixes

* restore Media Kit components in Ads wizard ([#3881](#3881)) ([86fa89a](86fa89a))
@matticbot

matticbot commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 6.2.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@dkoo dkoo mentioned this pull request Apr 7, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@laurelfulford laurelfulford laurelfulford approved these changes

@thomasguillot thomasguillot thomasguillot approved these changes

Assignees

@dkoo dkoo

Labels

released [Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dkoo @thomasguillot @matticbot @laurelfulford