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(ia-epic): Adjustments to Menu for Information Architecture UI #487

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

ronchambers
Copy link
Collaborator

@ronchambers ronchambers commented Oct 21, 2024

All Submissions:

Changes proposed in this Pull Request:

This code helps to complete Asana Task: "Newspack Information Architecture > IA Listings".

See the primary PR here: Automattic/newspack-plugin#3483

The code in this PR does 3 things:

  1. It adds a helpful note above a callback function to alert future developers that the callback function might be removed via remove_action by the "parent" Newspack Plugin. This could be helpful to know if a developer isn't seeing the correct output from the callback they expect.

  2. It adds a "todo" that recommends we remove the Category and Tag links since they just link to the normal Posts categories and tags anyway. This isn't needed in the main PR above, but in an effort to possibly "clean up" the plugin a bit, since it's getting cleaned up via the Information Architecture (IA) project anyway :-)

  3. The final change, is the one that I highly recommend we do. It will make the code in the main PR above easier. The change here is to move "Marketplace Listings" above "Generic Listings". In the IA project it was deemed that Marketplace should go above Generic. So my recommendation is to do a little "clean up" in the Listings plugin now and move Marketplace above Generic :-)

Screenshot:

listings-plugin-recommendations

How to test the changes in this Pull Request:

  1. Pull this branch.
  2. View the menu in wp-admin.

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?

@ronchambers ronchambers changed the title feat(ia-epic): adjustments to menu for Information Architecture UI feat(ia-epic): Adjustments to Menu for Information Architecture UI Oct 24, 2024
@ronchambers ronchambers marked this pull request as ready for review October 24, 2024 04:57
@ronchambers ronchambers requested a review from a team as a code owner October 24, 2024 04:57
@ronchambers
Copy link
Collaborator Author

@miguelpeixe - please see above. I'll assign you in Asana and 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.

Thanks, @ronchambers!

Changes look good and are consistent with what IA proposes. I agree we don't need duplicate taxonomy links here, so I went ahead and pushed 5b23399.

],
'rewrite' => [ 'slug' => $prefix . $settings['newspack_listings_generic_slug'] ],
],
// Move Marketplace above Generic here so that admin submenu will show Marketplace above Generic.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Move Marketplace above Generic here so that admin submenu will show Marketplace above Generic.

I don't think this comment is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @miguelpeixe for removing the category/tax links. As for the comment // Move Marketplace above Generic ... I agree the comment can be removed. I put it there because "everywhere else" in the plugin Generic is coded above Marketplace. But I'm totally cool with whatever you decide :-)

@miguelpeixe miguelpeixe merged commit 22b70d4 into trunk Oct 31, 2024
9 checks passed
@miguelpeixe miguelpeixe deleted the feat/ia-epic branch October 31, 2024 14:09
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.

2 participants