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

Deprecate Theme Switch modal #73726

Closed
wants to merge 2 commits into from
Closed

Conversation

miksansegundo
Copy link
Contributor

@miksansegundo miksansegundo commented Feb 23, 2023

Related to #69687
Site Onboarding Flows Homepage Setup > Switching to another theme PCYsg-Ln8-p2
Theme Switch User Research pekYwv-uV-p2
Theme Experience Walkthrough > Issues for preserving templates pekYwv-BQ-p2
Persist template content on theme switch pbxlJb-2X4-p2

Proposed Changes

  • Remove the theme switch modal
  • Pass dont_change_homepage: true in the request POST /themes/mine

Description

In this PR, we always pass dont_change_homepage: true in the request for a theme switch, which is the same as choosing the "Switch theme, preserving my homepage content" option in the modal.

We are forcing users to preserve their homepage content on theme switches, but we might have exceptions when testing.

Where is the logic for POST /themes/mine?

  • Simple sites fbhepr%2Skers%2Sjcpbz%2Schoyvp.ncv%2Serfg%2Sjcpbz%2Qwfba%2Qraqcbvagf%2Spynff.jcpbz%2Qwfba%2Qncv%2Qnpgvir%2Qgurzr%2Qraqcbvag.cuc%3Se%3Q1p463405%2330-og
  • Atomic sites fbhepr%2Skers%2Swrgcnpx%2Scebwrpgf%2Scyhtvaf%2Swrgcnpx%2Swfba%2Qraqcbvagf%2Swrgcnpx%2Spynff.wrgcnpx%2Qwfba%2Qncv%2Qgurzrf%2Qnpgvir%2Qraqcbvag.cuc%3Se%3Q3n3p4nqp%2336-og

TODO

  • Add a modal to explain exactly what will happen, including what will happen to their "in-template" content of their current theme and a mention to their reading settings.
  • Update the field guide page: Site Onboarding Flows Homepage Setup > Switching to another theme PCYsg-Ln8-p2

Known issues

Testing Instructions

The expected result after a theme switch is that the content on the homepage (front of the site), excluding template parts (e.g. header/footer), must be preserved.

Currently when you activate a new theme, any of the old theme templates that you have customised are retained. For this reason, even though template parts of the homepage template are not preserved after switching to another theme, they will be back if we switch back to the old theme.

The homepage outcome after a theme switch depends on the following:

  • the previous homepage templates edited before the switch
  • the site reading settings before the switch
  • the theme you switch from
  • the theme you switch to

Test flows

Flow 1 "Skip"

  • Create a new site
  • Skip to dashboard
  • Switch theme from Appearances -> Themes multiple times

Flow 2 "Pick a design"

  • Create a new site
  • Pick a theme from the design picker
  • Switch theme from Appearances -> Themes multiple times

Flow 3 "Assembler"

  • Create a new site
  • Click "Start designing" from the bottom of the design picker
    • Flow 3.1
      • Add patterns
    • Flow 3.2
      • Don't add patterns
    • Continue
  • Switch theme from Appearances -> Themes multiple times

Test buttons

From the theme detail, click "Activate this design"

Screenshot 2566-02-24 at 18 17 36

From the theme showcase, open the three dots menu and click "Activate"

Screenshot 2566-02-24 at 18 17 21

Test themes switch by theme type

  • Classic theme => Classic theme
  • Classic theme => Block theme
  • Block theme => Block theme
  • Block theme => Classic theme

Test themes switch by homepage type

Classic themes

Classic themes with posts as homepage

  • e.g.: Dyad 2, Independent Publisher 2, Ixion

Classic themes that uses a page as homepage

  • e.g.: Alves, Twenty Seventeen

Block themes

Block themes with posts as homepage

  • whose homepage template is "Index"
    • e.g.: Zoologist, Geologist, Rainfall, Masu
  • whose homepage template is "Home"
    • e.g.: Pendant / Archeo / Skatepark

Block themes that uses a page as homepage

  • whose homepage template is "Header and Footer Only"
    • e.g.: Appleton / Dorna / Russell
  • whose homepage template is "Footer Only"
    • e.g.: Ames

Test sites

From wordpress.com/themes/{ site slug }

  • Simple (free)
  • Atomic (business)
  • Jetpack (jurassic.ninja + connection)

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-ajp-p2)?

@miksansegundo miksansegundo requested a review from a team February 23, 2023 14:45
@miksansegundo miksansegundo self-assigned this Feb 23, 2023
@miksansegundo miksansegundo requested review from a team and worldomonation as code owners February 23, 2023 14:45
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 23, 2023
@matticbot
Copy link
Contributor

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

App Entrypoints (~141 bytes removed 📉 [gzipped])

name           parsed_size           gzip_size
entry-stepper       -401 B  (-0.0%)     -141 B  (-0.0%)
entry-main          -164 B  (-0.0%)      -68 B  (-0.0%)
entry-login         -164 B  (-0.0%)      -68 B  (-0.0%)

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

Sections (~4114 bytes removed 📉 [gzipped])

name              parsed_size           gzip_size
theme                 -8799 B  (-1.8%)    -1948 B  (-1.4%)
themes                -8429 B  (-1.1%)    -1951 B  (-0.9%)
checkout              -1320 B  (-0.1%)     -275 B  (-0.1%)
site-purchases         -237 B  (-0.0%)      -66 B  (-0.0%)
scan                   -237 B  (-0.0%)      -63 B  (-0.0%)
purchases              -237 B  (-0.0%)      -66 B  (-0.0%)
plugins                -237 B  (-0.0%)      -70 B  (-0.0%)
pages                  -237 B  (-0.0%)      -66 B  (-0.0%)
marketplace            -237 B  (-0.0%)      -86 B  (-0.0%)
hosting                -237 B  (-0.0%)      -66 B  (-0.0%)
gutenberg-editor       -237 B  (-0.0%)      -68 B  (-0.0%)
customize              -237 B  (-0.1%)      -69 B  (-0.1%)
backup                 -237 B  (-0.0%)      -68 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.

Async-loaded Components (~205 bytes removed 📉 [gzipped])

name                                                      parsed_size           gzip_size
async-load-signup-steps-theme-selection                        -237 B  (-0.1%)      -69 B  (-0.1%)
async-load-design-blocks                                       -237 B  (-0.0%)      -69 B  (-0.0%)
async-load-calypso-blocks-product-purchase-features-list       -237 B  (-0.5%)      -67 B  (-0.6%)

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

@github-actions
Copy link

github-actions bot commented Feb 23, 2023

const customizeUrl =
( isAtomic || isJetpack ) && hasAutoLoadingHomepage
? addQueryArgs(
{ 'new-homepage': true },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm removing this part because the param ?new-homepage=true is meant to open the modal in the Post editor to choose a starter page template for a new homepage, assuming Atomic and Jetpack sites don't support auto-loading-homepage behavior. However, Atomic sites support it. See #65976.

In this PR, we always pass dont_change_homepage: true, which skips the auto-loading-homepage behavior and that works on Atomic & Jetpack. See #65846 (comment) and fbhepr%2Skers%2Sjcpbzfu%2Ssrngher%2Qcyhtvaf%2Snhgbybnq%2Qubzrcntr%2Qercynprzrag.cuc%3Se%3Q923495oq%2320-og

I'll test these scenarios again to confirm we are safely removing that param.

@@ -29,33 +22,9 @@ export function activate(
siteId,
source = 'unknown',
purchased = false,
keepCurrentHomepage = false
keepCurrentHomepage = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm setting keepCurrentHomepage: true as default here, but it looks too fragile. We can also set dontChangeHomepage: true as default in the action activateTheme:

What do you think?

@miksansegundo miksansegundo force-pushed the remove/theme-switch-modal branch from 5f357a0 to 9aa33fd Compare February 24, 2023 04:07
@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit remove/theme-switch-modal on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@@ -29,33 +22,9 @@ export function activate(
siteId,
source = 'unknown',
purchased = false,
keepCurrentHomepage = false
keepCurrentHomepage = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

badge
Should we pass the persist_homepage_template: true always as well, or not?
fbhepr%2Skers%2Sjcpbz%2Schoyvp.ncv%2Serfg%2Sjcpbz%2Qwfba%2Qraqcbvagf%2Spynff.jcpbz%2Qwfba%2Qncv%2Qnpgvir%2Qgurzr%2Qraqcbvag.cuc%3Se%3Q1p463405%2333-og

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out.

I missed that persist_homepage_template is always false because the themes/theme-switch-persist-template feature flag is false in all environments.

That means the logic related to preserving the home template is not being used.

See pbxlJb-2X4-p2#comment-2162 and the changes in that API fbhepr%2Suvfgbel%2Sjcpbz%2Schoyvp.ncv%2Serfg%2Sjcpbz%2Qwfba%2Qraqcbvagf%2Spynff.jcpbz%2Qwfba%2Qncv%2Qnpgvir%2Qgurzr%2Qraqcbvag.cuc-og

@miksansegundo
Copy link
Contributor Author

miksansegundo commented Mar 19, 2023

Some interesting conversations on the theme switch and the template persistence

Here some of the comments:

User flows

First we should outline user flows that we anticipate or want to make possible. For example:

  1. Choosing a theme and wanting to switch everything to the new look (discard any user modifications in templates or styles).
  2. Wanting to preserve some customizations such as colors or header navigation. Or conversely; wanting to replace styles and navigation but leave everything else intact.
  3. Wanting to preserve everything (essentially incorporating a new theme's templates or patterns on demand).
  4. There could be themes with different set of templates (say, a commerce theme that brings in new templates) that are additive.

What is clear is that we can no longer assume for users what they may want to do but we should choose a good default.

It'd be reasonable to make the first option the default since it's what most are accustomed to, but we can discuss it. Semantic template parts are also a tool we have for managing some expectations since they can overlap between themes.

Theme switching should have the option of being a more granular operation in the site editor, walking users through some steps where they could choose to retain or discard specific templates (like a header) or global styles. Right now there's a lack of clarity upon switching themes about whether a specific part of the site will be replaced or not.

User flow:

I create a blank.html template that only renders the post content block (just like BlockBase). I've assigned it to display on a particular page, as I want no headers or footers — just my block content for this landing page. I change my theme to get a new look cause I feel like it, but now my landing page has a header, footer and an empty sidebar next to it.

To resolve this today, I need to create a blank custom template and assign it to the page once again. If I ever change my theme, I end up with the same issue, needed to create the blank template again (while deleting the old one).

User next steps:

While we can't assume that every time I change a theme, all my custom templates should stay, I do lean in the direction that that behavior should be the default experience.

Maybe we have a mechanism that let's me opt to resetting all templates as the theme's default templates, instead of what happens today where everything resets when a theme is changed.

In addition, I should also be able to use any template I've created on any theme. I'm not sure what the purpose behind locking templates to themes is, but its seems counter-intuitive.

Block templates are theme specific but that is not obvious.

I agree this deserve to be made clearer, maybe in the welcome guide? (Open to ideas here).

I wanted to detail the reasoning more here. The block templates are not necessarily theme specific, they can easily translate to other themes but theme CSS design can have impact and alter the result. In the future, we do want to allow them to be theme-agnostic but for the initial release we wanted to keep them theme specific because otherwise, there’s a lot of UX/UI work that need to happen on theme switching and elsewhere to take into account these new possibilities.

It’s also a decision based on the initial version of FSE themes we want to ship. FSE themes rely on these templates as well (wp_template) and we don’t want to solve the cross-themes templates in the first release but we want to start exploring this at their own pace. Keeping the template editor theme specific allow us to simplify the mental model of these templates: These are just editable “Page Templates” (the custom page templates themes provide)

@miksansegundo
Copy link
Contributor Author

miksansegundo commented Apr 25, 2023

@okmttdhr for the Live Preview spike, you might find relevant the details about the Theme switch that is on the description and comments in this PR. Feel free to reach out to me to discuss anything related to it.

Here some interesting threads:
p1677166541732979/1677165514.716649-slack-C048CUFRGFQ
p1677204510072609-slack-C048CUFRGFQ

The work on this PR has been stopped because removing the modal won't fix anything and the feedback from HEs was that the modal must explain exactly what will happen after the theme switch. The answer to that question is complex because there are several factors that can break the preview and the theme switch. So, in order to tell users what will happen we first must understand what is failing and that implies IMO understanding and fixing the preview and the theme switch.

@github-actions github-actions bot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 25, 2023
@okmttdhr
Copy link
Member

Thank you for your sharing, @miksansegundo!
You are definitely one who is familiar with theme-switching. I will ask everything after I catch up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants