-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Coming soon: Send public_coming_soon
site creation flag in development builds
#45428
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~161 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
79853fb
to
4f2ba2b
Compare
28aaa96
to
211ec35
Compare
@@ -74,6 +74,7 @@ export function* createSite( | |||
font_headings: selectedFonts.headings, | |||
} ), | |||
use_patterns: isEnabled( 'gutenboarding/use-patterns' ), | |||
public_coming_soon: isEnabled( 'gutenboarding/public-coming-soon' ) || undefined, |
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 will need to be updated to the new flag name wpcom_public_coming_soon
.
Is the || undefined
necessary? that would be the same behavior as leaving it off wouldn't it?
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 will need to be updated to the new flag name
wpcom_public_coming_soon
.
This is an API param so doesn't actually need to map directly to the site option. But maybe leaving out the wpcom_
when the name is so similar just makes it confusing.
Is the
|| undefined
necessary?
It might not be. I wanted to leave out the param from the payload if it's disabled, but I didn't actually test that the || undefined
was necessary.
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.
Just to be clear sending public_coming_soon=false
would still do the correct thing, I kinda just wanted to leave it off entirely though.
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 saw the existing code was doing the ...( someBool && { ... } )
trick to leave out some options. So I switched to that instead.
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.
ahh ok I see why it's neccessary 👍
); | ||
|
||
if ( isEcommerce ) { | ||
return 1; // Public and discoverable |
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 think you should use a named constant here instead of the number and comment e.g.
return PRIVACY_MODES.PUBLIC_AND_DISCOVERABLE
I see that the API defines the meaning of those numbers but it would be better to have something more formal here I think
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.
Good call. I could define those constants in the @automattic/data-stores
package and use them in the API type definition too.
8c1f45a
to
7a25d18
Compare
9f64d7e
to
3077113
Compare
I think this is good to go. @lsl @roo2 Any objections to enabling the coming soon feature flag? ✅ Create a site with any plan other than ecommerce ✅ Were you redirected to the correct place after gutenboarding? 🆗 |
@@ -34,7 +34,9 @@ export function* createSite( | |||
username: string, | |||
languageSlug: string, | |||
bearerToken?: string, | |||
isPublicSite = false | |||
visibility: number = isEnabled( 'gutenboarding/public-coming-soon' ) | |||
? Site.Visibility.PublicNotIndexed |
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.
Is this defaulting sites to the not indexed option?
e: swapped out on launch maybe?
e2: never mind caught up now #45019
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.
Yep. This default argument is never actually used (a value is always passed it), but sites in coming soon mode start as unindexed.
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.
The setting will actually come from here: https://github.com/Automattic/wp-calypso/pull/45428/files#diff-6be59baa71efc91a49a8e47a21563362R55
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.
Yep I've tested this with and without the feature flag and with ecommerce plan ✅ and the code looks good 👍
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, LGTM
Ecommerce sites aren't currently private-by-default, and also shouldn't use the new coming soon feature.
To silence warning that broke unit tests
The site was being created before the `useSelectedPlan` hook could update, which meant the plan passed to the `createSite` action wasn't up to date.
3077113
to
b994df8
Compare
Changes proposed in this Pull Request
?public-coming-soon
is present and onwpcalypso.wordpress.com
or a development buildpublic_coming_soon
flag to/sites/new
endpointThe
public_coming_soon
API option was merged in D49202-code, and it just sets thewpcom_public_coming_soon
site option.Part of #45019
Testing instructions
localhost:3000/new?flags=gutenboarding/public-coming-soon
?flags
query param will disappear as you go through the flow, but that's ok/sites/new
/sites/new
included apublic_coming_soon=true
option.?flags=gutenboarding/public-coming-soon
flag set, it should still be created public and indexable?flags
flag set, it should behave as before