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: Information Architecture for Newsletters #1678

Closed
wants to merge 5 commits into from
Closed

Conversation

ronchambers
Copy link
Collaborator

@ronchambers ronchambers commented Oct 21, 2024

All Submissions:

Changes proposed in this Pull Request:

This PR supports the main PR ( Automattic/newspack-plugin#3490 ) in order to clean up the existing Newsletters menu items when the Information Architecture (IA) project is released.

The current Newsletter menu and header can be seen in this screenshot:

newsletters-menu-and-header

  • Newsletters has a unique Newspack header at the top.
  • The menu has an "Add New" link that can also be found on the "All Newsletters" screen.
  • The menu links to what looks like "Newsletters" categories and tags, but they are the same categories and tags for Posts.

My recommendation is to remove menu items: Add New, Categories and Tags. Though, this is NOT a requirement for the Information Architecture Project. The IA Wizard code in the main PR (link above) already hides these links, so nothing is required here. Mainly it's just a recommendation to make the Newsletters plugin closer to what IA looks anyway 😃

You'll also see some "Note" comments added to the docblocks for callback functions that the IA code will "replace/remove". These comments are not required, but they might be helpful for developers if they are expecting a callback to run, but the IA Wizard in the Newspack Plugin is actually removing or replacing it.

Finally, there is one "todo" in the includes/class-newspack-newsletters-settings.php file in the get_settings_url() function. I haven't had the time to test that. I'd recommend that a team member who knows how the Newsletters plugin works should verify that function is working correctly when the IA Newsletters Wizard is active (see main PR link above).

How to test the changes in this Pull Request:

This pull request is mainly some "recommendations" and code comments, but there is one thing to test. In order to test the "todo" for the "get_settings_url()" (see paragraph above), please do the following:

  1. Go to the main PR link above and follow it's testing steps.
  2. Verify that the "get_settings_url()" works properly with the main PR active.

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?

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.26%. Comparing base (cdf2e24) to head (a4e6178).
Report is 24 commits behind head on trunk.

Additional details and impacted files
@@            Coverage Diff            @@
##              trunk    #1678   +/-   ##
=========================================
  Coverage     20.26%   20.26%           
  Complexity     2658     2658           
=========================================
  Files            48       48           
  Lines         10607    10607           
=========================================
  Hits           2149     2149           
  Misses         8458     8458           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ronchambers
Copy link
Collaborator Author

@miguelpeixe - I assigned you in Asana. I'll reach out in slack.

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.

For now, there's no need to make changes to this plugin that are not required for the IA project.

We can address changes here when unifying the settings page using newspack components, yet to be determined.

I'm going to close this, but feel free to reopen if there's more to it!

@miguelpeixe miguelpeixe closed this Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants