-
Notifications
You must be signed in to change notification settings - Fork 799
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
Add inline module settings for plan welcome page #8985
Conversation
Hi, I tested this against jurassic.ninja sites (before and after the change) and it looked and worked well. The only issue I came across was trying to activate ads on a personal site - it gave me an error: I had a look at both the pricing page, and the plans page (in Calypso) and couldn't find monetization as part of the personal plan so maybe this doesn't belong in this change? Otherwise everything else worked well for me for the other plans |
Fixed! Thanks for the review @alisterscott |
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.
I left a couple of comments on code. The two blockers that I see are that:
- We're adding a component that we're not using
- We're mention Jetpack Professional in all of the plan in the monitor akismet backups section.
I have a couple of other questions:
-
Since the
social-seo-ads-prompt.jsx
is only required for the Professional plan, and since it's specific to the Professional plan, should we just add toggles and phrasing directly into theprofessional.jsx
file? -
Similar thing for
social-ads-prompt.jsx
and that only being used in thepremium.jsx
file. Should we just roll that directly into thepremium.jsx
file?
return <div> | ||
<img src={ imagePath + 'security.svg' } className="jp-welcome__svg" alt={ __( 'Security' ) } /> | ||
<p> | ||
{ __( 'Jetpack Professional gives you everything you need to keep your hard work safe, including ' + |
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.
This is shown for all three plans. Perhaps we could say, "Your Jetpack plan...". Or, we can add a placeholder in and then substitute the plan name in.
@@ -0,0 +1,28 @@ | |||
/** |
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.
Are we using this component anywhere? I didn't notice it in the screenshots and when I search wordads-prompt
on this page, nothing shows up.
class VideoPressPrompt extends React.Component { | ||
render() { | ||
return <div> | ||
<img src={ imagePath + 'wordads.svg' } className="jp-welcome__svg" alt={ __( 'VideoPress' ) } /> |
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.
Since this icon is the same one we're using for the social and ads section, does it make sense to group the settings together under a single instance of the icon?
Re-tested and I'm no longer seeing the ads on a personal plan - thanks for the fix. As @ebinnion points out above, could the |
@ebinnion I removed the unused modules, moved modules only used in one place to be inline, fixed the wording about "Professional" plan to be more generic, and removed the icon from the VideoPress section (since it didn't quite fit in the other section, and there was no videopress-suitable svg icon already in the repo) |
Any chance you could take another look?
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.
Works and looks great!
Other than the copy tweak @alisterscott mentioned, it's good to go!
@gravityrail this one needs a rebase now |
Merged with master and added toggle for enabling Search. Here's a screenshot - I put the search toggle right below the first mention of Search in the landing page. This might not be the most elegant way to show it (or perhaps we need an icon above the para/toggle?) |
Looks great to me for a first iteration. Let's ship this! |
@gravityrail I started with a site from scratch, connected, then upgraded to personal... Was presented with the toggles. three of them. Two of them were toggled. Only the backups one was off. So i toggled it, and then it activated but the welcome screen closed itself unexpectedly. Is this ok ? |
Forget it. I can't reproduce. Tested all plans and looks great. |
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.
LGTM!
Note that the changes here, will not take advantage of the string updates done to the Learn More popover introduced in #9137. cc @eliorivero @rickybanister |
@rickybanister @eliorivero @keoshi I'm merging this one. Please let me know if we need to tackle the strings I mentioned on my previous comment |
Yeah, I think for consistency this should be implemented. |
* 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
Fixes #5909
Changes proposed in this Pull Request:
Testing instructions:
Screenshots
I have deliberately done the smallest change I can, while still sharing code across plans (which changes some copy). I have only removed prompts from the bottom of the form where I feel that there was a clearly redundant CTA.
Personal
Premium
Professional