-
Notifications
You must be signed in to change notification settings - Fork 799
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
Add an image thumbnail to wp-admin post list. #20720
Conversation
263260f
to
1bee4c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I had a few comments below.
I couldn't get the build to work on my local machine, for some reason. This is the error I got:
pnpm run build
> [email protected] build /Users/jeherve/code/jetpack/projects/packages/post-list
> wp-scripts build src/index.js
TypeError: Cannot destructure property 'info' of 'TerserPlugin.getAsset(...)' as it is undefined.
at TerserPlugin.taskGenerator (/Users/jeherve/code/jetpack/node_modules/.pnpm/[email protected][email protected]/node_modules/terser-webpack-plugin/dist/index.js:183:7)
at taskGenerator.next (<anonymous>)
I think it would be really good to have tests for all methods in this new package, especially since it plays with post content and fetches specific parts of it.
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking good. I think it would it be worth us slimming this down in order to get the skeleton of the package working and merged.
I wonder whether we should disable (or perhaps just by default, with an option to turn it on via Screen Options) the post thumbnail when the theme doesn't have support for them? It's rare, but we could call get_theme_support
to check. There's an argument that we should display the post's image regardless of theme support of course.
We should probably check that the post type supports featured images though via post_type_supports( $post_type, 'thumbnail' )
, but that might be at the point we use the package, rather than in the library code.
projects/packages/post-list/src/components/featured-image/index.js
Outdated
Show resolved
Hide resolved
2a7440e
to
93aa539
Compare
Already fixed when updating the |
@@ -0,0 +1,21 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a some discussion about using the Config package for handling this on the other PoC PR.
I think it would be a good plan to not have the package autoload like this, so it would be worth investigating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kraftbj 👋 We've been trying out your suggestion to use the Config package here, but we're not seeing any examples of the packages being loaded on wpcom. Once we've followed Adding your package to the config class, what's the next step?
projects/packages/post-list/src/components/featured-image/index.js
Outdated
Show resolved
Hide resolved
bcf7c8f
to
269008e
Compare
Since we removed the post-list dependency from the plugins composer file, I don't think we need this changelog file.
Updated initialization hook code. Capitalized words in headers.
142db64
to
c307802
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of minor suggestions.
Tested and it works as expected. :-)
✅ Set the post thumbnail via featured-image
✅ Set the post thumbnail via the first mage into the post content
Thanks!
Co-authored-by: Damián Suárez <[email protected]>
@jeherve How does it look? Are we mergeable? -- Thanks for all your help on this PR. 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Let's merge this! :) 🚢
Thanks, folks. Let me merge it for you, @DustyReagan |
We've created a Composer package in Jetpack to add an image thumbnail to the post-list wp-admin page similar to Calypso. For the thumbnail, we use the "featured image" if one exists, else we look in the post itself for the first media-library hosted image.
Design
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Setup Environment
jetpack install -r && jetpack install --all
watch
in case you'd like to edit the code in the client.Compiling
Production
Development
Development watching mode 👀
Unit Tests
To run the package unit tests:
cd projects/packages/post-list
composer test-php
.If you're running the PHP interpreter that shipped with your Mac you may get the error:
/usr/bin/php declares an invalid value for PHP_VERSION
. StackOverflow has the solution here.Load The Post List Package
To load the post list package you need to call it from the "Must Use Plugins" directory. To do that:
0-functions.php
in thetools/docker/mu-plugins/
directory.add_action( 'admin_init', array( '\Automattic\Jetpack\Post_List\Post_List', 'configure' ) );
Test Feature
Note that post-list=true must be set in the URL.