-
Notifications
You must be signed in to change notification settings - Fork 805
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 a launch site button to the admin bar. #41240
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
} else if ( function_exists( '\WPCOM\Lib\Launch_Site\is_launched' ) ) { | ||
$is_launched = \WPCOM\Lib\Launch_Site\is_launched( $blog_id ); | ||
$blog_domain = preg_replace( '!^https?://!', '', get_primary_redirect( $blog_id ) ); | ||
} |
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.
Finding the right APIs to use has been challenge. I'm not sure I'm covering all the subtle use-cases either.
I would love if we had a "codex" or "dev docs" to know which APIs are available to which sites, otherwise, it feels like we're just hacking things together.
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 would love if we had a "codex" or "dev docs" to know which APIs are available to which sites
That would be great indeed. In practice, we currently need to do some digging or look at prior art to get familiar with newer things.
In this specific use-case, there is prior art with #36839, where as you can see, they sticked to the same option check in both environments.
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.
Do you know if there's a consistent way to get the "domain name" as well (between atomic and simple) to avoid the condition.
@@ -0,0 +1,3 @@ | |||
#wpadminbar .launch-site { | |||
background: var(--wp-admin-theme-color); |
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've used the admin color CSS variable to try to stay consistent with the admin bar but I noticed that these variables don't contain the right values. I think this is out of scope of the current PR. I know some folks are working on making sure the right colors are used everywhere.
$is_launched = true; | ||
$blog_id = get_current_blog_id(); | ||
|
||
if ( defined( 'IS_ATOMIC' ) && IS_ATOMIC ) { |
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 you use the Status
package in that file already, you could switch to using is_woa_site
from Status\Host
$is_launched = get_option( 'launch-status' ) === 'launched'; | ||
$blog_domain = ( new Status() )->get_site_suffix(); | ||
} else if ( function_exists( '\WPCOM\Lib\Launch_Site\is_launched' ) ) { | ||
$is_launched = \WPCOM\Lib\Launch_Site\is_launched( $blog_id ); |
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.
When you run into PhanUndeclaredFunction
or other Phan errors, this can be a good help:
pdWQjU-Jb-p2
} else if ( function_exists( '\WPCOM\Lib\Launch_Site\is_launched' ) ) { | ||
$is_launched = \WPCOM\Lib\Launch_Site\is_launched( $blog_id ); | ||
$blog_domain = preg_replace( '!^https?://!', '', get_primary_redirect( $blog_id ) ); | ||
} |
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 would love if we had a "codex" or "dev docs" to know which APIs are available to which sites
That would be great indeed. In practice, we currently need to do some digging or look at prior art to get familiar with newer things.
In this specific use-case, there is prior art with #36839, where as you can see, they sticked to the same option check in both environments.
projects/packages/jetpack-mu-wpcom/src/class-jetpack-mu-wpcom.php
Outdated
Show resolved
Hide resolved
acd5992
to
276927b
Compare
* | ||
* @param WP_Admin_Bar $admin_bar The WordPress admin bar. | ||
*/ | ||
function wpcom_add_launch_button_to_admin_bar( WP_Admin_Bar $admin_bar ) { |
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.
We need to check whether the user is the admin of the site. Currently, the launch button shows up for everyone, even logged-out users, which is unexpected.
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.
Isn't the admin bar only shown for logged-in users. Also if I'm not wrong, if your site is unlaunched, no one is going to see the site (it's going to show the coming soon message) no?
I guess in other words, how do I reproduce the issue?
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 how we can reproduce the issue:
- Create a new unlaunched SIMPLE site.
- Patch this PR to the sandbox.
- Add a new user as Editor/Author.
- Log in to WordPress.com as that user (e.g. in another browser window).
- Open the site frontend as that user.
- You will see the admin bar with the launch button, which shouldn't be there. (Clicking the button will not launch the site)
Isn't the admin bar only shown for logged-in users.
Yeah, sorry, it won't be shown logged-out users. (Except if you're seeing an ATOMIC site frontend while PROXIED, which is a weird case and can be ignored). But the above case still applies -- we need to check for the current user's permission.
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.
So, I was actually unable to reproduce this issue. The Editor/Author can't see the frontend of the website and only see a "coming soon" page for me, so there's no admin bar.
That said, I pushed a code to add a check. Some small defensive coding is not that bad.
projects/packages/jetpack-mu-wpcom/src/class-jetpack-mu-wpcom.php
Outdated
Show resolved
Hide resolved
Nice. We should consider adding a confirmation dialog to clarify what launching entails. This will help users determine if they’re truly ready to proceed while providing a seamless and tasteful opportunity to upgrade their site. If launching is intentional and meaningful, it can also serve as a well-timed upsell moment (that's persistent if left "unlaunched". I'll write up some thoughts on this front. |
I think this is already the case, launching send you to a wizard to pick a domain/plan if you didn't pick one yet. For the confirmation, are you thinking about a regular browser confirm dialog? To be honest, I'm not entirely sure about this interaction. The launch wizard is already something you can abort at any time. |
$blog_id = get_current_blog_id(); | ||
$blog_domain = preg_replace( '!^https?://!', '', get_primary_redirect( $blog_id ) ); | ||
} | ||
if ( $is_launched ) { |
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.
Nit: you could return earlier.
$blog_domain = ( new Status() )->get_site_suffix(); | ||
} else { | ||
$blog_id = get_current_blog_id(); | ||
$blog_domain = preg_replace( '!^https?://!', '', get_primary_redirect( $blog_id ) ); |
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.
You can use wp_parse_url( home_url(), PHP_URL_HOST )
I think?
This is what the site management widget currently uses (untouched by me).
Also I think it's using that for both simple and atomic.
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 copied this code from the previous launch bar we had in the frontend but I can try your suggestion. There's so many ways of doing the same things and no reference for these things. I think we need this reference at some point.
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.
Tested and works for me. It's a bit weird that it sends you to the same flow as the one for creating a new site, but it has nothing to do with this PR.
4d43e69
to
fbf1307
Compare
Closes Automattic/wp-calypso#98895.
Proposed changes:
We recently removed the "launch bar" from the frontend of wp.com websites, that said, it's important for users to know that there's site is yet to be launched for users as raised by @scruffian
The current PR adds a "launch site" button to the admin bar of both simple and atomic sites.
Testing instructions:
Does this pull request change what data or activity we track or use?
No