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.

Sign up for GitHub

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

Jump to bottom

Load the Search widget even if Extra Sidebar Widgets is disabled #9062

Merged
merged 4 commits into from
Mar 26, 2018
Merged

Load the Search widget even if Extra Sidebar Widgets is disabled #9062

merged 4 commits into from
Mar 26, 2018

Conversation

Viper007Bond
Copy link
Contributor

@Viper007Bond Viper007Bond commented Mar 16, 2018 •
edited
Loading

Fixes #9041

Not 100% sure this is the best way to always load a widget or not. Suggestions welcome for alternate code choices.

Changes proposed in this Pull Request:

Always load the Search widget, even if the Extra Sidebar Widgets module is disabled. It's already loaded even if the Search module isn't loaded.

Force the Search widget to be loaded via the Search module so that even if Extra Sidebar Widgets is disabled, the widget is still available for use.

If both the Search module and the Extra Sidebar Widgets module are disabled, then the widget will not be loaded.

Testing instructions:

  1. Make sure your test site has a Professional plan.
  2. Visit /wp-admin/admin.php?page=jetpack_modules and disable both the Extra Sidebar Widgets module and Search module.
  3. Visit Appearance -> Widgets and make sure that "Search (Jetpack)" is not listed.
  4. Visit the Customizer and repeat the above confirmation.
  5. Enable the Search module. Verify that the Search widget and only the Search widget is available and that it's called "Search (Jetpack)".
  6. Disable the Search module.
  7. Enable the Extra Sidebar Widgets module. Verify that the Search widget and all of the other Jetpack widgets are available.

@Viper007Bond Viper007Bond added [Feature] Extra Sidebar Widgets [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] Search For all things related to Search labels Mar 16, 2018
@Viper007Bond Viper007Bond added this to the 6.0 milestone Mar 16, 2018
@Viper007Bond Viper007Bond self-assigned this Mar 16, 2018
@Viper007Bond Viper007Bond requested a review from a team as a code owner March 16, 2018 04:01
zinigor
zinigor previously approved these changes Mar 16, 2018
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Technically I have no problem with this - I have tested it and examined the code, everything is fine.

But, do we want to make Search a special widget like this? Perhaps a better way would be to help the user to activate sidebar widgets using JITMs and Settings notices?

@zinigor zinigor added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 16, 2018
@Viper007Bond
Copy link
Contributor Author

The idea was to allow people to start use the enhanced search functionality without having to go enable it first (using the widget would enable it). But yeah, perhaps a JITM or something would be better. "Hey, you have a plan that supports better search. Want to enable it?"

One alternative I can think of is to only load the widget if Extra Sidebar Widgets is enabled OR the Search module is enabled. That still fixes the original issue (widget disappearing if Extra Sidebar Widgets is disabled) without always loading it.

@zinigor
Copy link
Member

zinigor commented Mar 19, 2018

Since we don't have module dependencies, having Search Widget on only when either of the modules is turned on feels good enough to me. I'm glad to help with this if needed.

@zinigor zinigor added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 19, 2018
@ghost ghost removed the [Status] In Progress label Mar 19, 2018
@Viper007Bond
Load the search widget via the search module.
620a68b 
Fixes case where widget not loaded if Extra Sidebar Widgets is disabled.
@Viper007Bond Viper007Bond changed the title Always load the Search widget even if Extra Sidebar Widgets is disabled Load the Search widget even if Extra Sidebar Widgets is disabled Mar 20, 2018
@Viper007Bond Viper007Bond added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Mar 20, 2018
@Viper007Bond Viper007Bond dismissed zinigor’s stale review March 20, 2018 00:35

Significant code changes, needs a re-review

@Viper007Bond Viper007Bond requested a review from a team March 20, 2018 00:35
@Viper007Bond
Copy link
Contributor Author

@zinigor I changed things up so that the widget is only loaded if either (or both) the Search module or the widgets module is loaded. Needs a re-review. 🙂

dereksmart
dereksmart approved these changes Mar 26, 2018
View reviewed changes
Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

Read/tested after refactor. LGTM

@dereksmart dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Mar 26, 2018
@Viper007Bond Viper007Bond merged commit 23d8c4d into master Mar 26, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 26, 2018
@Viper007Bond Viper007Bond deleted the fix/always-load-search-widget branch March 26, 2018 22:41
dereksmart added a commit that referenced this pull request Mar 27, 2018
@dereksmart
add #9062 to changelog
be13ed7
dereksmart pushed a commit that referenced this pull request Mar 27, 2018
@oskosk @dereksmart
Changelog 6.0: running changelog (#9034)
f54fc7b 
* Changelog 6.0: create base for changelog.

* Add #8938 to changelog

* Add #8962 to changelog

* Add #8974 to changelog

* Add #8975 to changelog

* Add #8978 to changelog

* Add #8867 to changelog

* Add #8937 to changelog

* Add #8961 to changelog

* Add #8855 to changelog

* Add #8944 to changelog

* Add #8973 to changelog

* Add #8977 to changelog

* Add #8979 to changelog

* Add #8980 to changelog

* Add #8982 to changelog

* Add #8983 to changelog

* Add #8984 to changelog

* Add #8986 to changelog

* Add #9005 to changelog

* Add #9010 to changelog

* Add #9012 to changelog

* Add #9021 to changelog

* Add #9022 to changelog

* Add #9056 to changelog

* Add #9061 to changelog

* Add #9079 to changelog

* Add #9080 to changelog

* Add #9088 to changelog

* Add #9096 to changelog

* Add #9097 to changelog

* Add #9100 to changelog

* Add #9107 to changelog

* Add #8969 to changelog

* Add #8993 to changelog

* Add #9003 to changelog

* Add #9031 to changelog

* Add #8945 to changelog

* Add #9052 to changelog

* Add #9058 to changelog

* Add #9066 to changelog

* Add #9076 to changelog

* Add #9053 to changelog

* Add #9108 to changelog

* Add #9135 to changelog

* Add #9148 to changelog

* Add #9125 to changelog

* Add #9137 to changelog

* Added testing instructions for 6.0.

* Added IS testing instructions, huge props to @tiagonoronha.

* Added #8498 to changelog.

* Added #8954 to changelog.

* Added #8985 to changelog.

* add #9027

* add #9112 to changelog

* add #9136 to changelog

* add #9102 to changelog

* add #9093 to changelog

* add #9062 to changelog

* add #9172 to changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dereksmart dereksmart dereksmart approved these changes

@zinigor zinigor zinigor left review comments

Assignees

@Viper007Bond Viper007Bond

Labels
[Feature] Extra Sidebar Widgets [Feature] Search For all things related to Search
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Viper007Bond @zinigor @dereksmart @matticbot