Skip to content

Enable all non-experimental modules on plugin activation#191

Merged
felixarntz merged 5 commits intoWordPress:trunkfrom
kirtangajjar:feature/default-module-activation
Mar 2, 2022
Merged

Enable all non-experimental modules on plugin activation#191
felixarntz merged 5 commits intoWordPress:trunkfrom
kirtangajjar:feature/default-module-activation

Conversation

@kirtangajjar
Copy link
Member

@kirtangajjar kirtangajjar commented Feb 28, 2022

Summary

Fixes #61. Enable all non-experimental modules on plugin activation.

Fix is quiet simple so there isn't much to explain here.

@kirtangajjar
Copy link
Member Author

kirtangajjar commented Feb 28, 2022

@eclarke1 Can you please add "Needs Review" and other relevant labels as well?

@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure labels Feb 28, 2022
@kirtangajjar kirtangajjar mentioned this pull request Feb 28, 2022
Closed
@felixarntz felixarntz added this to the 1.0.0-beta.1 milestone Feb 28, 2022
@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature and removed [Type] Enhancement A suggestion for improvement of an existing feature labels Feb 28, 2022
felixarntz
felixarntz reviewed Feb 28, 2022
View reviewed changes
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.

@kirtangajjar This looks like a solid start. I left a few comments and suggestions for parts that need iteration.

felixarntz
felixarntz approved these changes Feb 28, 2022
View reviewed changes
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.

@kirtangajjar I've added those few requested changes and with that this looks good to me. Thank you for the PR! 🎉

mitogh
mitogh reviewed Feb 28, 2022
View reviewed changes
Copy link
Member

@mitogh mitogh left a comment

Choose a reason for hiding this comment

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

Overall looks great, few minor comments.

@kirtangajjar @mitogh
Update admin/load.php
f80fdac 
Bail if option is set with existing value

Co-authored-by: Crisoforo Gaspar Hernández <[email protected]>
@kirtangajjar
Copy link
Member Author

kirtangajjar commented Mar 1, 2022

Thank you @felixarntz @mitogh for your reviews. I have committed the suggestion made by @mitogh. Please do let me know incase this needs something else from my end :)

@felixarntz
Copy link
Member

felixarntz commented Mar 1, 2022

@kirtangajjar Sorry, see my comment in #191 (comment), I'm not sure that change actually makes sense. From what I can tell it was better the way you originally had it:

  • We want to only run our logic if the option is not set.
  • But now after the change suggested by @mitogh the logic is also run if the option is set to e.g. an empty array.
  • The latter is problematic since it would mean we may override the user-defined configuration just because they disabled all modules, which is unexpected UX.

felixarntz
felixarntz requested changes Mar 1, 2022
View reviewed changes
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.

@kirtangajjar One thing remaining here per the above.

cc @mitogh

@kirtangajjar @felixarntz
Update admin/load.php
d8495cc 
Co-authored-by: Felix Arntz <[email protected]>
@kirtangajjar kirtangajjar requested a review from felixarntz March 2, 2022 14:03
@kirtangajjar
Copy link
Member Author

kirtangajjar commented Mar 2, 2022

@felixarntz Fixed. Sorry didn't think that one through fully.

felixarntz
felixarntz approved these changes Mar 2, 2022
View reviewed changes
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 @kirtangajjar for the PR - awesome work! 🙌

@felixarntz felixarntz merged commit b483ca7 into WordPress:trunk Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixarntz felixarntz felixarntz approved these changes

@mitogh mitogh mitogh approved these changes

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein

@JustinyAhin JustinyAhin Awaiting requested review from JustinyAhin

@tillkruss tillkruss Awaiting requested review from tillkruss

@ThierryA ThierryA Awaiting requested review from ThierryA

Assignees

No one assigned

Labels

Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature

Projects

None yet

Milestone

1.0.0-beta.1

Development

Successfully merging this pull request may close these issues.

Define default module activation (onboarding)

4 participants

@kirtangajjar @felixarntz @mitogh @eclarke1