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

Highlight AMP-compatible themes and plugins in admin #6597

Merged

Conversation

dhaval-parekh
Copy link
Collaborator

Summary

Fixes #2313

AMP-GH-Issue-2313.mov

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2021

Plugin builds for 49b37fa are ready 🛎️!

@lgtm-com
Copy link

lgtm-com bot commented Sep 9, 2021

This pull request introduces 1 alert when merging 0cc9223 into 3f0146e - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@jwold
Copy link
Collaborator

jwold commented Sep 9, 2021

@dhaval-parekh I ran into an issue trying to test this. Just to clarify, does this also work when searching plugins and themes?

@dhaval-parekh dhaval-parekh self-assigned this Sep 10, 2021
@lgtm-com
Copy link

lgtm-com bot commented Sep 10, 2021

This pull request introduces 1 alert when merging 2df393c into deba08a - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@westonruter westonruter added this to the v2.2 milestone Sep 10, 2021
@dhaval-parekh dhaval-parekh changed the title Enhancement/2313 highlight amp compatible theme plugin Highlight amp compatible theme plugin Sep 10, 2021
@lgtm-com
Copy link

lgtm-com bot commented Sep 10, 2021

This pull request introduces 1 alert when merging af2799a into f819294 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@dhaval-parekh
Copy link
Collaborator Author

@dhaval-parekh I ran into an issue trying to test this. Just to clarify, does this also work when searching plugins and themes?

@jwold I think previously it was a bug and not showing. PX message after serach. But it's fixed now.

@westonruter
Copy link
Member

Regarding #6597 (comment):

WordPress uses 1000 ms as debounce wait time on keyup input event of the input search box. Within that function is store jQuery promise of AJAX request int wp.update.searchRequest global variable.

To ensure global variable wp.update.searchRequest our callback has to run after WordPress's callback, 1500ms is used as wait time in our callback.

Reference

OK, I think I see. So the debouncing must be at least 1001 milliseconds because before that there is no guarantee that wp.update.searchRequest will be defined. However, as soon as the request completes the wp.update.searchRequest variable is deleted bt core (unfortunately), so that means if the Ajax request completes in faster than 500ms, the 1500ms delay will run too late and wp.update.searchRequest will have been deleted. On my local system, the Ajax plugin search request takes ~800ms, so it works for me. However, on the dev environment for amp-wp.org the request returns in ~400ms, so this would fail to inject in that case (as 1000ms + 400ms < 1500ms).

I think what may be required is to use MutationObserver here instead for more reliability. I'll try.

* Override theme view.
*/
overrideViews() {
wp.themes.view.Theme = ampViewTheme;
Copy link
Member

Choose a reason for hiding this comment

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

Something we may need to look out for here: what if another plugin is also doing a similar thing? Will their overrides get lost? I suppose not if they are also extending wp.themes.view.Theme because if they do so then they would be extending our extension, or vice-versa. Still, something to look out for.

$response->themes[ $i ] = (object) $theme;
}
} else {
$response->themes = $themes;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to ever be called.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, We can remove that. But need to make sure the function only serve when $action is 'query_themes'.
I have updated it in the new PR #6681

*/
public function filter_plugins_table_api_args() {

$per_page = 100; // @todo There are currently 56 plugins, so this will show all. This is done because pagination is not working.
Copy link
Member

Choose a reason for hiding this comment

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

@dhaval-parekh Please look into pagination of the AMP-compatible plugins list in another PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please have a look at this PR #6681
I have addressed the pagination issue in this PR.

@westonruter westonruter merged commit c50cb9f into develop Nov 3, 2021
@westonruter westonruter deleted the enhancement/2313-highlight-amp-compatible-theme-plugin branch November 3, 2021 04:22
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highlight themes and plugins that are AMP-compatible inside WordPress
5 participants