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

Add search to the Professional plan warm welcome #8945

Merged
merged 1 commit into from
Mar 26, 2018
Merged

Add search to the Professional plan warm welcome #8945

merged 1 commit into from
Mar 26, 2018

Conversation

Viper007Bond
Copy link
Contributor

@Viper007Bond Viper007Bond commented Feb 28, 2018 •
edited by oskosk
Loading

Fixes #8694.

Changes proposed in this Pull Request:

#8694 (comment)

2018-02-27_19-27-52

Testing instructions:

  1. Comment out this return to get the dialog to show up: https://github.com/Automattic/jetpack/blob/master/_inc/client/components/welcome-new-plan/index.jsx#L50
  2. Run yarn build to build a fresh set of the JavaScript app.
  3. Visit the Jetpack dashboard in wp-admin on a site that has the Professional plan. Check out the updated dialog box.
  4. Verify that the new link at the bottom takes you to the Customizer's widget section.
  5. Verify that the Tracks event triggers when you click on that new link.

Changelog entry

Added Search to the features listed on the Warm Welcome dialog for Professional Plan

@Viper007Bond Viper007Bond added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] Search For all things related to Search [Feature] Warm Welcome labels Feb 28, 2018
@Viper007Bond Viper007Bond added this to the 5.9 milestone Feb 28, 2018
@Viper007Bond Viper007Bond self-assigned this Feb 28, 2018
@Viper007Bond Viper007Bond requested review from a team, gibrown and richardmuscat February 28, 2018 03:35
@Viper007Bond Viper007Bond added the Admin Page React-powered dashboard under the Jetpack menu label Feb 28, 2018
gibrown
gibrown requested changes Feb 28, 2018
View reviewed changes
Copy link
Member

@gibrown gibrown left a comment •
edited by Viper007Bond
Loading

Choose a reason for hiding this comment

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

Looks pretty good to me.

I got this brief error when clicking the link:

screen shot 2018-02-28 at 10 59 01 am

Quickly disappears because of the page load. I am guessing this is either because I have that one line commented out, or because I already had search enabled.

Request URL:[removed]/index.php?rest_route=/jetpack/v4/settings
Request Method:POST
Status Code:400 Bad Request

I also seem to get this when I just try to close the dialog.

Other than this, I would approve the PR. Can you reproduce?

@Viper007Bond
Copy link
Contributor Author

Hmm, yeah, I can reproduce and I'm not sure what's up. 🤔

@zinigor zinigor modified the milestones: 5.9, 6.0 Mar 6, 2018
@oskosk
Copy link
Contributor

oskosk commented Mar 16, 2018

I couldn't reproduce that warning message when starting from scratch and acquiring a Pro Plan

oskosk
oskosk approved these changes Mar 16, 2018
View reviewed changes
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

LGTM!

@oskosk oskosk 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 16, 2018
zinigor
zinigor previously requested 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.

I can reproduce the problem by running this code on a site that has Extra Sidebar Widgets disabled, so it's a manifestation of #9041. And the error seems to be happening because the settings endpoint can not find the respective setting.
Should we first enable Extra Sidebar Widgets here?
Also, it may be a red herring, but it looks like the request gets cancelled because of the page load, I can't even debug it in the dev toolbar.

gibrown
gibrown approved these changes Mar 20, 2018
View reviewed changes
Copy link
Member

@gibrown gibrown left a comment

Choose a reason for hiding this comment

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

I think that since the bug I raised is covered by that other issue (which has another PR to fix it) that we should merge this as is.

@oskosk oskosk dismissed zinigor’s stale review March 26, 2018 14:03

Proposal is to let #9062 fix the issue

@oskosk oskosk merged commit 71a776d 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
@oskosk oskosk deleted the update/warm-welcome/search branch March 26, 2018 14:03
oskosk added a commit that referenced this pull request Mar 26, 2018
@oskosk
Add #8945 to changelog
2d7fdc1
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

@gibrown gibrown gibrown approved these changes

@oskosk oskosk oskosk approved these changes

@zinigor zinigor zinigor left review comments

@richardmuscat richardmuscat Awaiting requested review from richardmuscat

Assignees

@Viper007Bond Viper007Bond

Labels
Admin Page React-powered dashboard under the Jetpack menu [Feature] Search For all things related to Search [Feature] Warm Welcome [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Viper007Bond @oskosk @zinigor @gibrown @matticbot