Dynamically change color scheme from color_scheme query param #96557

moon0326 · 2024-11-19T20:00:18Z

Related to woocommerce/woocommerce#52875

Proposed Changes

Support changing the color scheme based on the color_scheme query parameter when the user is using the new passwordless Jetpack connection

Testing Instructions

Checkout this branch locally and run yarn start
Once the build is ready, visit this link
Confirm the color scheme is sunrise
Copy the link and search for color_scheme
Change it to one of the supported color schemes and revisit the link.
Confirm the color scheme change.

Supported color schemes:

default
light
modern
blue
coffee
ectoplasm
midnight
ocean
sunrise

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests 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-aUh-p2)?

github-actions · 2024-11-19T20:00:32Z

Calypso Live (direct link)

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

Jetpack Cloud live (direct link)

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

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-125207&env=a8c-for-agencies

matticbot · 2024-11-19T20:01:22Z

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

App Entrypoints (~36 bytes added 📈 [gzipped])

name           parsed_size           gzip_size
entry-stepper       +956 B  (+0.0%)      +36 B  (+0.0%)
entry-main          +956 B  (+0.0%)      +36 B  (+0.0%)
entry-login         +956 B  (+0.0%)      +36 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~10 bytes added 📈 [gzipped])

name              parsed_size           gzip_size
purchase-product        +29 B  (+0.0%)      +10 B  (+0.0%)
jetpack-connect         +29 B  (+0.0%)      +10 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.

chihsuan

Tested well! 👍 I have some comments on the code.

chihsuan · 2024-11-20T01:41:03Z

client/layout/color-scheme/index.jsx

+export function getColorSchemeFromCurrentQuery( currentQuery ) {
+	return currentQuery?.color_scheme
+		? currentQuery?.color_scheme
+		: new URLSearchParams( currentQuery?.redirect_to ).get( 'color_scheme' );


If redirect_to is null, undefined, or an invalid URL, URLSearchParams will throw an error. Should we add a fallback for cases where redirect_to is not a valid URL?

chihsuan · 2024-11-20T01:48:55Z

client/layout/index.jsx

+		let colorScheme = null;
+
+		// Try to get the color scheme from `color_scheme` query param if we're in the WooCommerce Passwordless JPC flow.
+		if ( isWooPasswordlessJPC ) {


How about using the ternary operator here to simplify the logic?

let colorScheme = isWooPasswordlessJPC ? getColorSchemeFromCurrentQuery(currentQuery) : getColorScheme({ state, isGlobalSidebarVisible, sectionName, });

I was tempted to use ternary at first.

When color_scheme query param is missing for some reason, I think we should call getColorScheme to get the user's preference instead.

I think we should call getColorScheme to get the user's preference instead.

That sounds good. I noticed there doesn't seem to be a global user preference on WPCOM anymore. We could perhaps also just default the standard WPCOM colors 🤔

@adrianduffell Good catch! Let's use the default color scheme in that case.

@chihsuan @adrianduffell Fixed it in Return 'default' color scheme

chihsuan · 2024-11-20T01:54:47Z

client/layout/logged-out.jsx

@@ -153,6 +156,10 @@ const LayoutLoggedOut = ( {

 	let masterbar = null;

+	useEffect( () => {
+		refreshColorScheme( 'default', colorScheme );


Should we check if isWooPasswordlessJPC before calling the function? If it's only needed for Woo.

@chihsuan FIxed it in c184c35

chihsuan · 2024-11-20T01:55:33Z

client/layout/masterbar/woo.scss

@@ -185,7 +185,7 @@ $breakpoint-mobile: 660px;

 		.progress-bar__progress {
 			border-radius: 0;
-			background-color: var(--wp-admin-theme-color);
+			background-color: var(--color-primary);


adrianduffell · 2024-11-20T14:51:50Z

client/layout/masterbar/woo.scss

+// Color scheme 'blue' fix
+// The 'blue' theme color from wp-calypso does not match the 'blue' color from the local WordPress admin color scheme.
+// Fix the color scheme 'blue' to match the local WordPress admin color scheme on the new JPC page.
+body.is-blue {


What color does wp-calypso render for the 'blue' scheme? I wonder if we might have it wrong on the WC core side -- I noticed when 'blue' is applied, WordPress renders buttons in a tan #d9aa59 color, but in core profiler they are #2a63a1.

@adrianduffell Good catch. I think the core profiler has incorrect color. We've been using --wp-admin-theme-color for the button color, but it should be --color-pirmary. I'll double check and open a PR in WC repo.

@adrianduffell

Good catch. I think the core profiler has incorrect color.

I take it back 😄

Without this change, wp-calypso renders the exact same color as the core profiler.

I've removed it in Remove blue color fix change -- no longer needed

adrianduffell

Thanks @moon0326, it is testing nice and smooth! Pre-approving once the comments are resolved 🚀

matticbot · 2024-11-20T16:41:32Z

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

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

notifications

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/52876-persist-user-admin-color-scheme-on-wpcom-jpc on your sandbox.

chihsuan

Thanks, LGTM!

* Change color theme by color_scheme query param * Use --color-primary for the primary button and the progress bar * Fix blue theme color to match the local WordPress * Apply color_scheme from query * Refresh color scheme when if it is woo passwordless JPC * Return 'default' color scheme * Remove blue color fix change -- no longer needed * Fix error with isWooPasswordlessJPC access

moon0326 added 4 commits November 18, 2024 16:31

Change color theme by color_scheme query param

63577d8

Use --color-primary for the primary button and the progress bar

41ae190

Fix blue theme color to match the local WordPress

07a69f0

Apply color_scheme from query

0b5d46c

moon0326 requested review from a team, chihsuan and adrianduffell November 19, 2024 20:00

matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 19, 2024

moon0326 mentioned this pull request Nov 19, 2024

Pass color_scheme param to jetpack connection for core profiler woocommerce/woocommerce#52925

Merged

12 tasks

chihsuan reviewed Nov 20, 2024

View reviewed changes

adrianduffell reviewed Nov 20, 2024

View reviewed changes

adrianduffell self-requested a review November 20, 2024 14:57

adrianduffell approved these changes Nov 20, 2024

View reviewed changes

moon0326 added 2 commits November 20, 2024 08:32

Refresh color scheme when if it is woo passwordless JPC

6caaed4

Return 'default' color scheme

ef3813f

moon0326 added 2 commits November 20, 2024 12:58

Remove blue color fix change -- no longer needed

89130bf

Fix error with isWooPasswordlessJPC access

c184c35

chihsuan approved these changes Nov 21, 2024

View reviewed changes

moon0326 merged commit 6142a40 into trunk Nov 21, 2024
11 checks passed

moon0326 deleted the update/52876-persist-user-admin-color-scheme-on-wpcom-jpc branch November 21, 2024 15:43

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 Nov 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamically change color scheme from color_scheme query param #96557

Dynamically change color scheme from color_scheme query param #96557

moon0326 commented Nov 19, 2024

github-actions bot commented Nov 19, 2024 •

edited by matticbot

Loading

matticbot commented Nov 19, 2024 •

edited

Loading

chihsuan left a comment

chihsuan Nov 20, 2024

chihsuan Nov 20, 2024

moon0326 Nov 20, 2024

adrianduffell Nov 20, 2024

moon0326 Nov 20, 2024

moon0326 Nov 20, 2024

chihsuan Nov 20, 2024

moon0326 Nov 20, 2024 •

edited

Loading

chihsuan Nov 20, 2024

adrianduffell Nov 20, 2024 •

edited

Loading

moon0326 Nov 20, 2024

moon0326 Nov 20, 2024

adrianduffell left a comment

matticbot commented Nov 20, 2024 •

edited

Loading

chihsuan left a comment

Dynamically change color scheme from color_scheme query param #96557

Dynamically change color scheme from color_scheme query param #96557

Conversation

moon0326 commented Nov 19, 2024

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions bot commented Nov 19, 2024 • edited by matticbot Loading

matticbot commented Nov 19, 2024 • edited Loading

chihsuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moon0326 Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianduffell Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianduffell left a comment

Choose a reason for hiding this comment

matticbot commented Nov 20, 2024 • edited Loading

chihsuan left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2024 •

edited by matticbot

Loading

matticbot commented Nov 19, 2024 •

edited

Loading

moon0326 Nov 20, 2024 •

edited

Loading

adrianduffell Nov 20, 2024 •

edited

Loading

matticbot commented Nov 20, 2024 •

edited

Loading