Skip to content
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 theme_color_override_with_admin_color_scheme flag and use to update meta theme color tag #98381

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Jan 14, 2025

Related to #98335

Needs #98380 to be merged first

Proposed Changes

  • Add a new theme_color_override_with_admin_color_scheme environment config option
  • Use it to enable/disable updating the <meta name="theme-color" /> tag dynamically on the client whenever a user updates the admin color scheme

Why are these changes being made?

To match the desired behavior described in #98335. This feature is currently broken, as the data-colorscheme attribute that the logic relies is not applied in the code anymore (probably removed my mistake in a previous refactor).

This PR reinstates the behavior, but it changes the logic: instead of relying on the theme-color being defined or not (which is fragile and unclear), it uses a dedicated environment config option. Not only this logic is more clear, but it is also compatible with the changes being proposed in #98380

Testing Instructions

  • Load calypso in any wordpress.com environments
  • Observe the initial value of the <meta name="theme-color" /> tag in the page
  • Change the admin color scheme (by visiting /wp-admin/profile.php)
  • Verify that the interface changes color dynamically as a color scheme is chosen
  • Save the user preferences and go back to calypso UI
  • Observe the new value of the <meta name="theme-color" /> tag and make sure it follows the main color of the new chosen theme
  • Change the theme_color_override_with_admin_color_scheme to false for the relevant environment (eg development), and make sure that the <meta name="theme-color" /> tag doesn't follow the admin color scheme
  • A4A and jetpack cloud environments should also keep the value of the <meta name="theme-color" /> tag fixed, ie. they should not update it to follow any changes from the admin color scheme.

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 UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • 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)?

@matticbot
Copy link
Contributor

matticbot commented Jan 14, 2025

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

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

name         parsed_size           gzip_size
entry-main         +67 B  (+0.0%)       +2 B  (+0.0%)
entry-login        +67 B  (+0.0%)       +2 B  (+0.0%)

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.
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.

@matticbot
Copy link
Contributor

matticbot commented Jan 14, 2025

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 fix/theme_color_update_admin_color_scheme_config_option on your sandbox.

@ciampo ciampo requested a review from taipeicoder January 15, 2025 17:56
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 15, 2025
@ciampo ciampo requested review from jkguidaven, fushar and a team January 15, 2025 17:56
@ciampo ciampo self-assigned this Jan 15, 2025
@ciampo ciampo marked this pull request as ready for review January 15, 2025 17:56
@ciampo
Copy link
Contributor Author

ciampo commented Jan 15, 2025

@tyxla also pointed out another environment config variable called me/account/color-scheme-picker which is potentially used to enable/disable color schemes although I don't have much context around it — @fushar @okmttdhr it looks like you may have more context on this, given the work on the "site level user profile". Could you advise? Thank you 🙏

@ciampo ciampo force-pushed the fix/theme_color_update_admin_color_scheme_config_option branch from 2ed13df to e0e5406 Compare January 15, 2025 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants