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

Implement migration logic and UI from Performance Lab modules to their standalone plugins #899

Merged
merged 15 commits into from Dec 18, 2023

Conversation

mukeshpanchal27
Copy link
Member

Summary

Fixes #652

Relevant technical choices

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature no milestone PRs that do not have a defined milestone for release Creating standalone plugins [Focus] Standalone Plugins labels Dec 11, 2023
@mukeshpanchal27 mukeshpanchal27 self-assigned this Dec 11, 2023
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thank you @mukeshpanchal27 for the PR. I think there are a few things that can be simplified here. Most importantly, let's remove the additional piece of functionality in the module cards which wasn't required and makes the PR more complicated. I left more specific feedback below. Please let me know if you have any questions for clarification.

admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/load.php Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
@felixarntz felixarntz added this to the PL Plugin 2.8.0 milestone Dec 12, 2023
@felixarntz felixarntz removed the no milestone PRs that do not have a defined milestone for release label Dec 12, 2023
@felixarntz felixarntz changed the title POC 652: Migration logic and UI from old PL modules to new standalone plugins in PL settings screen Implement migration logic and UI from Performance Lab modules to their standalone plugins Dec 12, 2023
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/perflab-admin.js Outdated Show resolved Hide resolved
@mukeshpanchal27
Copy link
Member Author

Thank you, @felixarntz and @westonruter, for the review feedback. I've simplified the logic, addressed all the feedback, and responded to some open questions. The PR is now ready for review.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 I left some follow up feedback, mostly minor things to clarify wording. For example, I think we should include in the user-facing message that the modules will be removed in the future, for a sense of urgency.

There's one larger consideration here that we may want to change, which I'm just realizing now:

  • You've implemented the migration logic in an AJAX request. While this works well, it comes with the problem that then the whole UI on the page may be outdated (e.g. after other plugins were installed, modules deactivated etc.).
  • It will probably be too much effort to fix all the UI throughout the page to reflect those changes. Therefore I would suggest to go with a regular full page refresh and instead of AJAX move the logic to an admin_action_{$action} callback, similar to how the plugin activation and deactivation currently is.
  • You should be able to copy the logic pretty much as is, just replace the wp_send_json_error() calls with equivalent wp_die(), and the wp_send_json_success() with a redirect back to the Performance Lab plugin screen.
  • Doing that will also drastically simplify this PR: You can probably remove the JS file entirely, and the button can simply be a link to e.g. wp-admin/plugins.php?action=...&_wpnonce=....
  • Because this is then a full page reload, we don't have to worry about all other UI on the page, as it will be automatically refreshed after the changes have been made.

Let me know what you think about this, or if you have any follow up questions. For now, it's probably best if you just address the more specific feedback below, and we can discuss the above separately afterwards (e.g. in Slack or in PR comments).

admin/load.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
mukeshpanchal27 and others added 2 commits December 14, 2023 14:54
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 I just tested this again, and I think it looks almost ready to merge.

Regarding my feedback in #899 (review), I just now realized that you have a window.location.reload() in the JS, so that works perfectly fine then, and the AJAX with loading spinner still makes for a bit nicer user experience. 🎉

My final bits of feedback are below, after that it should be good to merge from my perspective.

admin/plugins.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/perflab-module-migration-notice.js Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
mukeshpanchal27 and others added 3 commits December 15, 2023 09:12
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@mukeshpanchal27
Copy link
Member Author

Regarding my feedback in #899 (review), I just now realized that you have a window.location.reload() in the JS, so that works perfectly fine then, and the AJAX with loading spinner still makes for a bit nicer user experience. 🎉

Thanks @felixarntz. In 388be6a i introduce perflab_replace_html_entity function that change html entriey as in alert it show server&#8217;s instead of server's

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @mukeshpanchal27!

admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated
}
}

$result = activate_plugin( WP_PLUGIN_DIR . '/' . $plugin_basename );
Copy link
Member

Choose a reason for hiding this comment

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

What if the plugin isn't located in WP_PLUGIN_DIR (even though this may be unlikely)? Also, the phpdoc for activate_plugin() says that this arg is relative not absolute.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fantastic catch! I completely overlooked that aspect. Thank you for pointing it out. I've made the necessary updates, and you can find them in the commit here: 2bd591d.

admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/plugins.php Outdated Show resolved Hide resolved
@swissspidy swissspidy dismissed westonruter’s stale review December 18, 2023 10:10

Feedback has been incorporated

@swissspidy swissspidy merged commit 4d88d57 into feature/creating-standalone-plugins Dec 18, 2023
8 checks passed
@swissspidy swissspidy deleted the wip/652 branch December 18, 2023 10:10
@felixarntz felixarntz added the Infrastructure Issues for the overall performance plugin infrastructure label Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Creating standalone plugins Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants