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

WP.com Posts: Show thumbnail on the posts list screen #19777

Closed
wants to merge 6 commits into from

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented May 7, 2021

Changes proposed in this Pull Request:

  • Introduces a new wpcom-posts package which seeks to be home for all the enhancements to apply on the Posts and Pages screens for WordPress.com sites.
  • This is part of a technical spike exploring how we can build a Composer package using the Jetpack monorepo which will be exclusively consumed by Simple and Atomic sites.
  • For now, this package simply displays the post thumbnail in the posts/pages list table (helps fixing Featured Images Next to Posts Missing wp-calypso#51357).

Screen Shot 2021-05-07 at 17 01 02

Remaining tasks:

  • Create package skeleton.
  • Implement logic for showing the thumbnail on a new column in the posts screen.
  • Add unit tests.
  • Create mirror repo.
  • Publish first version of package.

Jetpack product discussion

pd2C0z-b-p2

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

  • Spin up a local Jetpack site.
  • Add the new package to the Jetpack plugin: cd projects/plugins/jetpack && composer require automattic/jetpack-wpcom-posts.
  • Go to Posts > All posts.
  • Make sure the featured image of the posts is displayed in a new column.

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2021

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ⚠️ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

@@ -0,0 +1,14 @@
This program is free software; you can redistribute it and/or
Copy link
Member

Choose a reason for hiding this comment

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

We haven't typically added licenses to our packages until now, but you make a good point. I've asked in p2y3YZ-4v3-p2

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, this was added by the Generate Wizard.

Copy link
Member

Choose a reason for hiding this comment

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

It seems @sdixon194 planned for it already 🥳

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup! I commented on @kraftbj 's issue here in case we needed to update it: #19802

"minimum-stability": "dev",
"prefer-stable": true,
"extra": {
"mirror-repo": "Automattic/jetpack-wpcom-posts",
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be okay to enable autotagger for this package.

Suggested change
"mirror-repo": "Automattic/jetpack-wpcom-posts",
"autotagger": true,
"mirror-repo": "Automattic/jetpack-wpcom-posts",

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't know about that, thanks! I guess we can maybe include an additional step in the Generate Wizard asking about this option, so it increases the visibility of this feature.

Copy link
Member

Choose a reason for hiding this comment

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

@sdixon194 Do you think we could add this to the generate command?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure thing, we can do that! Opened an issue to track it here: #19816

@@ -0,0 +1,21 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is something that should be within the Jetpack plugin instead, or within wpcomsh, and then this package would only be the package itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or something that is used with the Config package.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to leave this out of the Jetpack plugin for various reasons:

  • This is part of a technical spike exploring how we can build a package which will be exclusively consumed by WP.com Simple and Atomic sites.
  • I don't see a clear fit for this feature in Jetpack (especially after branding it as a solution for Security, Backups, Performance, and Growth). Even if we decide there is a fit, there is not certainty that all future features that will be bundled in this package may fit.

I do see value in setting up this via the Config package (which will be done on the WP.com sites), so I'll explore that approach.

@mmtr
Copy link
Member Author

mmtr commented May 12, 2021

Putting this on hold, since we decided to stop investigating this approach in order to better assign our resources during the next 3 months. Further reading: pd2C0z-b-p2#comment-23

@jeherve
Copy link
Member

jeherve commented Mar 2, 2022

Closing this for now because of the lack of activity on this. We can always reopen in the future if needed.

@jeherve jeherve closed this Mar 2, 2022
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.

5 participants