-
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
Refactor post list thumbnail preview to function server side. All javascript removed. #21126
Conversation
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. |
Hi, @JanaMW27! Can we get your perspective on this: Should we hide thumbnails on mobile view or always keep the thumbnails visible?
|
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.
Only some minor comments really. This is working well for me, and feels better than using JS for this feature. Good job!
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.
… action out of enqueue_scripts and into current_screen.
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.
Nice. This is working well for me and we're handling RTL properly too.
I just had a minor thought about moving where we register one of the hooks. I was thinking that we could keep the check to see what screen we're on to one place, but it's just a thought - take it or leave it :)
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
Just FYI, I did the following steps to add this feature to the Jetpack plugin
git checkout update/post-list-thumbnail-preview
cd projects/plugins/jetpack && composer require automattic/jetpack-post-list
Add post_list
into the features array:
foreach (
array(
'sync',
'jitm',
'post_list',
)
as $feature
) {
$config->ensure( $feature );
}
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.
LGTM
…r side. All javascript removed. (#21126) * Reworking post-list thumbnails to be server side. * Added changelog file. * Added CSS and placeholder image for empty thumbnails. * CSS fix placeholder disappearing. * Updated unit tests. Added alt tag to thumbnail image. * Updated comments. * Added rtl.css. * Don't add thumbnail column if 'title' is missing for some reason. * Add thumbnails to "Pages" admin table. Removed unused class name from thumbnail img. * Created add_thumbnail_filters_and_actions and moved column filter and action out of enqueue_scripts and into current_screen. * Removed unused in_admin_footer action.
Refactor post list thumbnail preview (originating from this PR) to function server side. All javascript removed.
This allows us to leverage the built in functionality of the screen options toggle.
Changes proposed in this Pull Request:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Setup Environment
jetpack install -r && jetpack install --all
Test Features
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.