-
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
Remove coming soon settings option for v2 #45896
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~198 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~50 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. 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. |
Checking for v1 coming soon mode flag so we can cater for existing coming soon users
03d932f
to
44aa622
Compare
Rebased, reverted the settings page and pushed up a bit of a commit cleanup to make changes more clear: https://github.com/Automattic/wp-calypso/pull/45896/files/4ff37845dac8bb5ea137069842ad2a98274e8ce9..44aa622177e8a3cb693ca5368cc685083405e24a. Split the settings page out in #46034 minus all the refactor cleanup, that should be safe to merge early as it doesn't change anything unless the flag is flipped on. There is a fair bit still in here to do with launching / site creation steps. Should we be looking at moving that stuff serverside into headstart by chance? |
@roo2 has something working over at #45950 using hooks, effectively bypassing headstart 🕺 As for launching, there's also an action we could hook into when a user launches a site, in the |
* This adds a coming soon page selector to the ellipsis menu. We're also showing a coming soon badge. * feature flag the coming soon badge * Switching flag to coming-soon-v2 to match #45896 Checking for v1 coming soon mode flag so we can cater for existing coming soon users * Remove the ability to set the coming soon page for free sites * Adding tracking to coming soon page id toggle
44aa622
to
8d4880f
Compare
f66de1c
to
e146fd8
Compare
There's good stuff in this PR, but I think we should write a clearer mission statement for what it is and isn't trying to achieve. That way it's going to be easier to know when this is mergeable. Can I propose a purpose of:
w.r.t (3) I'm thinking that most things will actually just work for v2 sites and nothing needs to be hidden, but investigating this is in scope for this PR w.r.t (2) this is the unclearest part of the PR for me. Have I even correctly summarised how we want the general settings form to work? |
Most of this is just iterations on the original PR ramon drafted #45167 with a few things pulled out so far, keen to split out more mergable parts if you can spot what they are.
Currently, If the v2 flag is enabled the coming soon settings page option will disappear once a user switches out of coming soon and hits save. It will show for any user with the coming soon v1 site option. v2 site option is completely ignored by the settings page. With pbxlJb-q6-p2#comment-538, This is changing in #46119, users will be able to change back into coming soon mode regardless of version, once v2 is enabled switching back and forth will upgrade your site to v2/public mode. |
Cool! Thanks for the clarification. I think this is a good plan. I was initially unclear whether coming-soon-v2 would re-use the same radio controls as v1, and this makes it clear that we currently don't expect the UI to change much between v1 and v2. |
The only thing that determines whether a site should be created as coming-soon-v2 is whether the feature flag is enabled. Not whether the site is private or not. This change also makes it possible for `getNewSitecomingSoonSettingV2()` to return 1. Before it would always return 0 no matter what.
This PR doesn't change the behaviour of `isSiteComingSoon` and I think this commit should have returned the tests back to testing the original behaviour. I've also tried to reduce the PR diff to make it a bit easier to read by putting some of the original code back and creating a seperate `describe` block for `isSiteComingSoonV2`.
e146fd8
to
36e17be
Compare
Changes proposed in this Pull Request
Testing instructions
Regardless of etk's/0-sandbox.php define:
define( 'WPCOM_PUBLIC_COMING_SOON', true );
http://calypso.localhost:3000/settings/general/<yourtestsite>.wo\rdpress.com?flags=coming-soon-v2
Should show existing coming soon settings, switching to public/private and saving settings should remove the option.
http://calypso.localhost:3000/settings/general/<yourtestsite>.wo\rdpress.com?flags=coming-soon-v2=false
Should leave things functioning the same as:
http://wordpress.com/settings/general/<yourtestsite>.wo\rdpress.com
props: @roo2 / @ramonjd (alot of code got squashed/rebased out of #45606