Skip to content

Conversation

@dkoo
Copy link
Contributor

@dkoo dkoo commented Oct 30, 2025

All Submissions:

Changes proposed in this Pull Request:

See Automattic/newspack-scripts#219.

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?

@laurelfulford
Copy link
Contributor

laurelfulford commented Oct 31, 2025

  • Run npm start or npm ci (no --legacy-peer-deps!)
  • Confirm that the install completes successfully
  • Run npm run build and npm run watch and confirm that the assets build successfully
  • Run npm run lint and confirm that SCSS and JS linting works
  • Run npm run test and confirm that JS unit tests work (only applicable if the repo has any) - returned "No JS unit tests in this repository."
  • Run npm run semantic-release --dry-run and confirm there's successful output
  • Smoke test both WP admin and front-end functionality—there should be no significant changes

Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

For the most part this looks good! I just spotted one odd thing when I went from the post editor to the brands (Post editor > Brands panel > Manage brands):

CleanShot 2025-10-31 at 14 44 02

That's this page: /wp-admin/admin.php?page=newspack-multi-branded-sites

If I go from Newspack > Settings > Additional Brands, it looks normal (that's wp-admin/admin.php?page=newspack-settings#/additional-brands).

When I follow the initial path on trunk (Post editor > Brands panel > Manage brands link), the page looks normal (with no gap, and with a white background rather than grey).

I can't see anything that stands out as the issue - there isn't a unique JS error on this branch that isn't present on trunk. I also get this when I'm also running the updated dependancies branch of the Newspack Plugin.

This feels minor (I'm not sure how often people go from the post editor to that screen) and the Multibranded settings are functional from there, though the gap/grey background persist on the other screens.

Just let me know if you have any questions about this!

@dkoo
Copy link
Contributor Author

dkoo commented Oct 31, 2025

@laurelfulford it took me a little while to figure out that the /wp-admin/admin.php?page=newspack-multi-branded-sites page is a different page than wp-admin/admin.php?page=newspack-settings#/additional-brands. It looks like the former one is supposed to be used when Newspack Plugin isn't active.

5b0ab82 adds a check and redirects the standalone page URL to the Newspack Plugin settings page if available, and updates the "Manage brands" link in the editor sidebar too. It also adds some light CSS fixes for the standalone page to make the background white and remove the gap, but really it's an edge case to use the plugin without the main Newspack Plugin, so I didn't focus too much on it.

@dkoo dkoo requested a review from laurelfulford October 31, 2025 23:10
Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

Thanks @dkoo! That was a weird one -- I thought it was a style thing since the page looked different on trunk and in this branch but that also didn't really make sense 🙂

This loos good now, and I double-checked the plugin without Newspack Plugin active for good measure. All the screens look great! 🚢

@dkoo dkoo marked this pull request as ready for review November 3, 2025 18:29
@dkoo dkoo requested a review from a team as a code owner November 3, 2025 18:29
@dkoo dkoo merged commit 8945900 into trunk Nov 3, 2025
5 checks passed
@dkoo dkoo deleted the chore/update-dependencies-oct-2025 branch November 3, 2025 18:29
@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Hey @dkoo, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants