-
Notifications
You must be signed in to change notification settings - Fork 15
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
Remove plugin-install-info in assets/js/admin/wc-setup.js
.
#224
Comments
Thanks for moving that comment to a new issue. |
On checking this issue, the logic used to leave this behind was that maybe the wizard might be used in future to install some recommended plugins. Should this be reconsidered? CC won't recommend plugins for install via the wizard like Woo? |
I don't see the point of having "suggested plugins" in the setup wizard at all. Everyone's shop is different so it's only ever going to be suggestions for a subset of users. For me the setup wizard is for putting the basics in place to get you up and running. I think all the plugin-related stuff should be on the dedicated Extensions page. That will also make it easier to maintain as we start getting more CC-specific options. |
It looked to me like this was just partially removed code and what was left behind wouldn't do anything on its own. If that is the case then I agree it should be removed entirely. I could have read this incorrectly though. |
I'm in favour of removing it. "Recommended" for whom? Personally speaking, I find recommendations annoying and the less informed user might feel obligated to install the plugin just because it's been recommended. |
I have been meaning to go through the PRs referenced here more closely but I haven't had time yet. I think there is still more to do here, since this code was removed pretty quickly, without a clear explanation of what each piece was doing.
There is also a bit more to remove. For example
plugin-install-info
appears several times inassets/js/admin/wc-setup.js
.Since a lot of what I'm thinking about checking here has already been done in previous PRs, I think this one is ok to merge for now, and we should follow up on this code removal separately.
Originally posted by @nylen in #204 (comment)
The text was updated successfully, but these errors were encountered: