Theme Showcase: Update Controllers for an "All Sites" View #87000

Copons · 2024-01-30T13:45:52Z

Fixes https://github.com/Automattic/dotcom-forge/issues/5383

Proposed Changes

Add a new selectSiteIfLoggedInWithOneSite routing callback to be used in logged-in /themes and /theme.

The new callback allows for a new "All Sites" view, accessible to users owning multiple sites, that defers the site selection to the latest possible moment, rather than before opening the Theme Showcase.

This change's purpose is to let users browse themes immediately instead of having to select a site first, using the old and unappealing site selector.

The new selectSiteIfLoggedInWithOneSite callback practically replaces the previous selectSiteIfLoggedInWithSites, introduced in #82917 in support of a "no sites" view. The old callback remains available, but it's currently not used anywhere.

After the change, we'll have the following routing situation:

Sites	Before	After	Notes
0			No changes
1			No changes
2+			"Pick theme" actions trigger a `start/with-theme` flow.

About Theme Actions and the `with-theme` Flow

This is only the first step of a larger project.

This PR is only responsible for changing routing.
Another PR will introduce a deferred site selection modal integrated with the various "Pick theme" (and similar) actions.

Testing Instructions

Use a test user with zero sites.
- Open /themes or /theme/bedrock (or any other Theme Details page) without site fragment in URL.
- Ensure both pages open without additional steps.
Use a test user with exactly one site.
- Open /themes or /theme/bedrock (or any other Theme Details page) without site fragment in URL.
- Ensure the routing automatically selects the only site, and both pages open without additional steps.
Use a test user with two or more sites.
- Open /themes or /theme/bedrock (or any other Theme Details page) without site fragment in URL.
- Ensure both pages open without additional steps.
- Try clicking on various theme links.
- Ensure they start a new onboarding flow (just like the zero sites example).

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
https://wpcalypso.wordpress.com/devdocs/docs/testing/index.md for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

github-actions · 2024-01-30T13:46:04Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-97642

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-97642&env=jetpack

matticbot · 2024-01-31T17:12:03Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~157 bytes added 📈 [gzipped])

name                               parsed_size           gzip_size
woocommerce-installation                 +67 B  (+0.0%)      +38 B  (+0.0%)
woocommerce                              +67 B  (+0.0%)      +38 B  (+0.0%)
themes                                   +67 B  (+0.0%)      +16 B  (+0.0%)
theme                                    +67 B  (+0.0%)      +16 B  (+0.0%)
subscribers                              +67 B  (+0.0%)      +18 B  (+0.0%)
stats                                    +67 B  (+0.0%)      +17 B  (+0.0%)
site-purchases                           +67 B  (+0.0%)      +19 B  (+0.0%)
site-monitoring                          +67 B  (+0.0%)      +38 B  (+0.0%)
settings-writing                         +67 B  (+0.0%)      +38 B  (+0.0%)
settings-security                        +67 B  (+0.0%)      +38 B  (+0.0%)
settings-reading                         +67 B  (+0.0%)      +38 B  (+0.0%)
settings-podcast                         +67 B  (+0.0%)      +38 B  (+0.0%)
settings-performance                     +67 B  (+0.0%)      +38 B  (+0.0%)
settings-newsletter                      +67 B  (+0.0%)      +38 B  (+0.0%)
settings-jetpack                         +67 B  (+0.0%)      +38 B  (+0.0%)
settings-discussion                      +67 B  (+0.0%)      +38 B  (+0.0%)
settings                                 +67 B  (+0.0%)      +17 B  (+0.0%)
scan                                     +67 B  (+0.0%)      +38 B  (+0.0%)
purchases                                +67 B  (+0.0%)      +19 B  (+0.0%)
promote-post-i2                          +67 B  (+0.0%)      +38 B  (+0.0%)
preview                                  +67 B  (+0.0%)      +38 B  (+0.0%)
posts-custom                             +67 B  (+0.0%)      +38 B  (+0.0%)
posts                                    +67 B  (+0.0%)      +38 B  (+0.0%)
plugins                                  +67 B  (+0.0%)      +17 B  (+0.0%)
plans                                    +67 B  (+0.0%)      +15 B  (+0.0%)
people                                   +67 B  (+0.0%)      +17 B  (+0.0%)
pages                                    +67 B  (+0.0%)      +38 B  (+0.0%)
migrate                                  +67 B  (+0.0%)      +38 B  (+0.0%)
media                                    +67 B  (+0.0%)      +18 B  (+0.0%)
marketplace                              +67 B  (+0.0%)      +17 B  (+0.0%)
marketing                                +67 B  (+0.0%)      +38 B  (+0.0%)
jetpack-social                           +67 B  (+0.0%)      +38 B  (+0.0%)
jetpack-search                           +67 B  (+0.0%)      +38 B  (+0.0%)
jetpack-connect                          +67 B  (+0.0%)      +38 B  (+0.0%)
jetpack-cloud-settings                   +67 B  (+0.0%)      +38 B  (+0.0%)
jetpack-cloud-pricing                    +67 B  (+0.0%)      +38 B  (+0.0%)
jetpack-cloud-plugin-management          +67 B  (+0.0%)      +17 B  (+0.0%)
jetpack-cloud-overview                   +67 B  (+0.0%)      +38 B  (+0.0%)
jetpack-cloud-features-comparison        +67 B  (+0.0%)      +38 B  (+0.0%)
jetpack-cloud                            +67 B  (+0.0%)      +38 B  (+0.0%)
import                                   +67 B  (+0.0%)      +38 B  (+0.0%)
hosting                                  +67 B  (+0.0%)      +17 B  (+0.0%)
home                                     +67 B  (+0.0%)      +15 B  (+0.0%)
gutenberg-editor                         +67 B  (+0.0%)      +38 B  (+0.0%)
google-my-business                       +67 B  (+0.0%)      +38 B  (+0.0%)
github-deployments                       +67 B  (+0.0%)      +38 B  (+0.0%)
export                                   +67 B  (+0.0%)      +38 B  (+0.0%)
email                                    +67 B  (+0.0%)      +38 B  (+0.0%)
earn                                     +67 B  (+0.0%)      +18 B  (+0.0%)
domains                                  +67 B  (+0.0%)      +19 B  (+0.0%)
customize                                +67 B  (+0.0%)      +38 B  (+0.0%)
concierge                                +67 B  (+0.0%)      +38 B  (+0.0%)
comments                                 +67 B  (+0.0%)      +38 B  (+0.0%)
checkout                                 +67 B  (+0.0%)      +19 B  (+0.0%)
backup                                   +67 B  (+0.0%)      +17 B  (+0.0%)
add-ons                                  +67 B  (+0.0%)      +38 B  (+0.0%)
activity                                 +67 B  (+0.0%)      +38 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

rcrdortiz · 2024-02-05T14:33:44Z

client/my-sites/controller.js

+	// If the user is logged out, has 0 sites, or the path contains a site fragment,
+	// proceed with the regular site selection.
+	if ( ! isLoggedIn || ! siteCount || !! siteFragment ) {
+		siteSelection( context, next );
+		return;
+	}


I don't think this matches with the name of the function. From the name I was expecting this to select the site if only one site was available or to proceed with the chain, and right now the function is doing 2 different things or proceeding the chain.

Yeah, I'm not very happy with both naming and flow, but I couldn't find a better idea. 🤔

Do you have a suggestion for making this cleaner?

We could extract this to a new command (maybe called selectSiteIfLoggedIn) that we will add to the chain after the selectSiteIfLoggedInWithOneSite command. That would make things cleaner.

Alternatively, we could just rename this to something more generic like handleSiteSelection.

I've renamed it as selectSiteOrSkipIfLoggedInWithMultipleSites.
It's a mouthful, but it's also descriptive of what it does. 🙂

mmtr

I think selectSiteIfLoggedInWithSites is no longer used after these changes, can we remove it?

mmtr · 2024-02-06T09:55:17Z

client/my-sites/controller.js

+	// If the user is logged out, has 0 sites, or the path contains a site fragment,
+	// proceed with the regular site selection.
+	if ( ! isLoggedIn || ! siteCount || !! siteFragment ) {
+		siteSelection( context, next );
+		return;
+	}


Alternatively, we could just rename this to something more generic like handleSiteSelection.

xavier-lc

Works as described

matticbot · 2024-02-14T12:11:36Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/themes-controller-all-sites on your sandbox.

Copons added [Status] In Progress [Feature] Theme Showcase The theme showcase screen in Calypso in Appearance > Themes. labels Jan 30, 2024

Copons self-assigned this Jan 30, 2024

Copons force-pushed the update/themes-controller-all-sites branch 2 times, most recently from 95af80d to d818654 Compare February 5, 2024 13:00

Copons requested review from arthur791004 and a team February 5, 2024 13:32

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 5, 2024

Copons removed the [Status] In Progress label Feb 5, 2024

rcrdortiz reviewed Feb 5, 2024

View reviewed changes

mmtr reviewed Feb 6, 2024

View reviewed changes

Copons force-pushed the update/themes-controller-all-sites branch from d818654 to 7e4bf1f Compare February 7, 2024 11:53

Copons mentioned this pull request Feb 7, 2024

Theme Details: Add a Site Selector Modal #87260

Merged

7 tasks

xavier-lc approved these changes Feb 14, 2024

View reviewed changes

Copons added 3 commits February 14, 2024 13:04

Update controllers to allow "all sites" on /themes and /theme

fe0e6c8

Remove unused selectSiteIfLoggedInWithSites function

67a15fb

Rename route callback

3a7f0fe

xavier-lc force-pushed the update/themes-controller-all-sites branch from 551efda to 3a7f0fe Compare February 14, 2024 12:05

xavier-lc merged commit 47b8a69 into trunk Feb 14, 2024
11 checks passed

xavier-lc deleted the update/themes-controller-all-sites branch February 14, 2024 12:19

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 14, 2024

xavier-lc mentioned this pull request Feb 15, 2024

Force no site selection on /theme and /themes #87463

Merged

7 tasks

TimBroddin pushed a commit that referenced this pull request Feb 20, 2024

Theme Showcase: Update Controllers for an "All Sites" View (#87000)

b2e30ba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme Showcase: Update Controllers for an "All Sites" View #87000

Theme Showcase: Update Controllers for an "All Sites" View #87000

Copons commented Jan 30, 2024 •

edited

Loading

github-actions bot commented Jan 30, 2024 •

edited by matticbot

Loading

matticbot commented Jan 31, 2024 •

edited

Loading

rcrdortiz Feb 5, 2024

Copons Feb 5, 2024

rcrdortiz Feb 6, 2024

mmtr Feb 6, 2024

Copons Feb 7, 2024

mmtr left a comment

mmtr Feb 6, 2024

xavier-lc left a comment

matticbot commented Feb 14, 2024

Theme Showcase: Update Controllers for an "All Sites" View #87000

Theme Showcase: Update Controllers for an "All Sites" View #87000

Conversation

Copons commented Jan 30, 2024 • edited Loading

Proposed Changes

About Theme Actions and the with-theme Flow

Testing Instructions

Pre-merge Checklist

github-actions bot commented Jan 30, 2024 • edited by matticbot Loading

matticbot commented Jan 31, 2024 • edited Loading

rcrdortiz Feb 5, 2024

Choose a reason for hiding this comment

Copons Feb 5, 2024

Choose a reason for hiding this comment

rcrdortiz Feb 6, 2024

Choose a reason for hiding this comment

mmtr Feb 6, 2024

Choose a reason for hiding this comment

Copons Feb 7, 2024

Choose a reason for hiding this comment

mmtr left a comment

Choose a reason for hiding this comment

mmtr Feb 6, 2024

Choose a reason for hiding this comment

xavier-lc left a comment

Choose a reason for hiding this comment

matticbot commented Feb 14, 2024

Copons commented Jan 30, 2024 •

edited

Loading

About Theme Actions and the `with-theme` Flow

github-actions bot commented Jan 30, 2024 •

edited by matticbot

Loading

matticbot commented Jan 31, 2024 •

edited

Loading