-
Notifications
You must be signed in to change notification settings - Fork 798
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
Introduce new wpl_iframe_src
filter to enable filtering arguments to Likes iframes.
#40276
Conversation
…o like iframes. This will enable experimenting with re-introducing `slim` likeboxes to display likes without gravatars and other future changes. Could also enable testing grounds of new changes before full UI for setting management is built out, or potentially to point more easily to a testing sandbox. Companion to proposed changes WPCOM-side via phab diff D165404
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Setting up the Jetpack CLI tool isn't functional for me currently. I've tried to follow the directions as best I could, but it seems
|
I had to do a minor change to package.json locally to get pnpm to install -- `"pnpm": "^9.3.0 <9.15.0"` but it worked.
I sorted this locally with the following change -- diff --git a/package.json b/package.json
index 347afccb62..bf8194e99f 100644
--- a/package.json
+++ b/package.json
@@ -35,7 +35,7 @@
},
"engines": {
"node": "^22.9.0",
- "pnpm": "^9.3.0 <9.12.0"
+ "pnpm": "^9.3.0 <9.15.0"
},
"packageManager": "[email protected]"
} Unsure if pnpm needs to have a max version required in package.json? |
|
What are some other changes that you're looking into with Likes? It would be important to have a path with these types of settings configurable in the Likes Block. |
Just what's outlined in the phab diff. Getting the "slim" mode up and running again, and retooling it to operate as the current full likes block behaves, but without gravatars. Longer term, I'd like to have to have that potentially available as a setting for the likes button exposed to admins -- whether or not to display gravatars of users who have liked it -- but I'm personally fine with that resolving down the road following any discussion / bikeshedding on whether to include it is finalized. |
@@ -448,6 +448,9 @@ function_exists( '\Activitypub\is_activitypub_request' ) | |||
|
|||
$title = esc_html__( 'Like or Reblog', 'jetpack' ); | |||
|
|||
/** This filter is documented in modules/likes/jetpack-likes-master-iframe.php */ | |||
$src = apply_filters( 'wpl_iframe_src', $src ); |
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.
Seems to me that only the likes/master.html
iframe needs the slim=1
parameter. On the likes/index.html
iframe the parameter has no effect, because the document is just a small and passive placeholder. All the work is done by the master iframe: it loads the likes from REST, decides whether the UI will be slim or not and then replaces the DOM markup in all the managed iframes.
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 had been shuffling some stuff in my testing and thought it was required there as well -- or it wasn't propagating properly without it. I could have been mistaken though.
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 used the existing pattern of wpl_ for the filter prefix but I'm 100% fine if it's changed to jetpack_likes_ for a filter prefix or anything else.
I think it may be better to switch to the jetpack_
prefix indeed. While wpl
is consistent with the rest of the module, it isn't with the rest of Jetpack, and the Plugins team now prefers prefixes to be clear and consistent all across plugins. Since this is a new prefix, we may as well switch to jetpack_likes
👍
projects/plugins/jetpack/modules/likes/jetpack-likes-master-iframe.php
Outdated
Show resolved
Hide resolved
…rame.php Co-authored-by: Jeremy Herve <[email protected]>
Just noting for folks testing this, the way to use this filter is as follows:
|
Thanks @jeherve! If this could go out in the December release, so that we can use the filter on production sites when the likes widget changeset merges to accept the slim parameter, it would be very much appreciated. |
Warning: one might be tempted to add the return add_query_arg( 'slim', '1', $src ) But that won't work because the |
This will enable experimenting with re-introducing
slim
likeboxes to display likes without gravatars and other future changes. Could also enable testing grounds of new changes before full UI for setting management is built out, or potentially to point more easily to a testing sandbox.Companion to proposed changes WPCOM-side via phab diff D165404
I used the existing pattern of
wpl_
for the filter prefix but I'm 100% fine if it's changed tojetpack_likes_
for a filter prefix or anything else.Proposed changes:
Other information:
Jetpack product discussion
I've been discussing this with @jsnajdr in Slack about the wpcom-side changes. Jetpack-side changes for now are limited to just adding two filters so it is possible to append a
slim=1
argument.Slim mode was added for the Carousel module eleven years ago, and removed again eight years ago via #3585 but the code supporting it has lived on wpcom-side, albeit in a broken state for the past few years. The proposed change wpcom-side rectifies that and lets slim mode work again as a version without gravatars. This PR to Jetpack just makes it possible to filter and append the iframe to add
&slim=1
to the fragment so JS picks it up and toggles the mode.Does this pull request change what data or activity we track or use?
Testing instructions: