-
Notifications
You must be signed in to change notification settings - Fork 802
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
Top Posts: allow one to customize avatar image options w/ filter #13117
Conversation
The jetpack_top_posts_widget_image_options filter allows third-parties to specify custom image options, including the fallback_to_avatars parameter. However we currently do not use the value that comes from the filter when we actually fetch the image. That fixes it.
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: August 6, 2019. |
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.
For some reason, I can not make gravatar image to disappear when using Image List
or Image Grid
display options. Text List
option does not display images by default. Maybe am I doing something wrong?
I tried adding jeherve_custom_top_posts_images
filter via Snippets plugin and into mu-plugin directly.
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 made it work with JN site. unfortunately, it seems it does not work as advertised:
- I can see a bunch of
PHP Warning: Illegal string offset 'src' in /srv/users/userf429d970/apps/userf429d970/public/wp-content/plugins/jetpack-dev/modules/widgets/top-posts.php on line 372
- widget code tries to render an image since it follows the
grid
/image
formatting, but since image is null - it renders broken image icon.
I think we should render a text-only
formatting in this case
@brbrr Yes, that is one of the reasons we have that fallback in the first place. If someone gets rid of that fallback, I think it's reasonable to expect that they would have another fallback in place instead, such as a function hooked into I think we can add a better handling of errors when no image can be found, but I think that belongs in a different 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.
Make sense. LGTM!
* Add initial changelog / testing list changes for 7.6 * Update stable tag to 7.5.3 * changelog: add #12957 * Changelog: add #12932 * Changelog: add #12867 * Changelog: add #12823 * changelog: add #12969 * changelog: add #13012 * changelog: add #12974 * Changelog: add #13059 * Changelog: add #13079 * Changelog: add #12924 * changelog: add #12954 * Changelog: add #12959 * Changelog: add #12977 * Changelog: add #12830 * Changelog: add #12926 * Changelog: add #12958 * Changelog: add #12999 * Changelog: add #13077 * Changelog: add #13083 * Changelog: add #13087 * Changelog: add #13110 * Changelog: add #13116 * Changelog: add #13117 * Changelog: add #12821 * Changelog: add #13120 * changelog: add #13139 * Changelog: add #13143 * Changelog: add #13147 * Testing list: add section about sync
* Add initial changelog / testing list changes for 7.6 * Update stable tag to 7.5.3 * changelog: add #12957 * Changelog: add #12932 * Changelog: add #12867 * Changelog: add #12823 * changelog: add #12969 * changelog: add #13012 * changelog: add #12974 * Changelog: add #13059 * Changelog: add #13079 * Changelog: add #12924 * changelog: add #12954 * Changelog: add #12959 * Changelog: add #12977 * Changelog: add #12830 * Changelog: add #12926 * Changelog: add #12958 * Changelog: add #12999 * Changelog: add #13077 * Changelog: add #13083 * Changelog: add #13087 * Changelog: add #13110 * Changelog: add #13116 * Changelog: add #13117 * Changelog: add #12821 * Changelog: add #13120 * changelog: add #13139 * Changelog: add #13143 * Changelog: add #13147 * Testing list: add section about sync
Changes proposed in this Pull Request:
The
jetpack_top_posts_widget_image_options
filter allows third-parties to specify custom image options, including the fallback_to_avatars parameter. However we currently do not use the value that comes from the filter when we actually fetch the image. That fixes it.Testing instructions:
Proposed changelog entry for your changes: