-
Notifications
You must be signed in to change notification settings - Fork 98
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
Conversation
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.
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.
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. |
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.
@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 equivalentwp_die()
, and thewp_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).
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
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.
@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.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Thanks @felixarntz. In 388be6a i introduce |
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.
Looks great, thanks @mukeshpanchal27!
admin/load.php
Outdated
} | ||
} | ||
|
||
$result = activate_plugin( WP_PLUGIN_DIR . '/' . $plugin_basename ); |
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.
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.
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.
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.
Feedback has been incorporated
4d88d57
into
feature/creating-standalone-plugins
Summary
Fixes #652
Relevant technical choices
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.