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

Theme Showcase: Add Atomic support for auto-loading homepage modal #65060

Closed
wants to merge 4 commits into from

Conversation

sixhours
Copy link
Contributor

@sixhours sixhours commented Jun 28, 2022

Proposed Changes

  • Following up on Andy's work, add the siteId param to themeHasAutoLoadingHomepage selector so we can check themes for the auto-loading-homepage tag whether on wpcom or individual Atomic sites
  • Remove checks for isJetpackSite/isAtomicSite when using the auto-loading-homepage action since we no longer want to exclude them.
  • Requires upcoming Jetpack changes that should be available to Atomic sites Wed. Jun 19 in Minor: Add action before switch theme jetpack#24818
  • This shouldn't be deployed until after we have the functionality ready in wpcomsh.

Testing Instructions

  • Switch to this PR, navigate to /themes on your Atomic site.
  • Clicking on a theme should pop up the auto-loading home page modal (if the theme supports it -- some themes to try are Appleton, Dorna, Winkel):

Screen Shot 2022-06-28 at 5 12 18 PM

  • Older themes (like Dara or Hexa) should not display the modal; they should instead just activate.
  • The actual homepage switch won't work until we use the new hook introduced in Minor: Add action before switch theme jetpack#24818 from within wpcomsh, which will require a separate diff.

Related to 997-gh-Automattic/wpcomsh and Automattic/jetpack#24818 to fix #56869

@sixhours sixhours added Atomic [Feature] Theme Showcase The theme showcase screen in Calypso in Appearance > Themes. labels Jun 28, 2022
@sixhours sixhours requested a review from a team June 28, 2022 20:20
@sixhours sixhours self-assigned this Jun 28, 2022
@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 Jun 28, 2022
@github-actions
Copy link

github-actions bot commented Jun 28, 2022

@matticbot
Copy link
Contributor

matticbot commented Jun 28, 2022

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

Sections (~48 bytes removed 📉 [gzipped])

name      parsed_size           gzip_size
themes         -225 B  (-0.0%)      -18 B  (-0.0%)
theme          -225 B  (-0.0%)      -30 B  (-0.0%)
checkout        +75 B  (+0.0%)      +39 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.

@sixhours
Copy link
Contributor Author

This isn't quite working as expected:

  • Even if the theme has the auto-loading-homepage tag, it's not available to the siteId via getTheme unless the site has activated the theme at least once, thereby adding it to its individual library.
  • Once the theme has been activated on the site, and if it has the auto-loading-homepage tag, the modal will work as expected.

So I think we need to fall back to checking against wpcom if the theme is not yet available under the Atomic siteId.

@andres-blanco
Copy link
Contributor

I've set up a new AT site and it works as expected! I will start working on the wpcomsh change 💪

@andres-blanco
Copy link
Contributor

andres-blanco commented Jul 12, 2022

An extra change to Jetpack needed to be issued to allow the dont_change_homepage argument in the /sites/%s/themes/mine endpoint. This is the PR.

@andres-blanco
Copy link
Contributor

@sixhours Do you think this should be enabled on Jetpack or only on Atomic? My initial guess was that this should only be enabled on Atomic.

@sixhours
Copy link
Contributor Author

Do you think this should be enabled on Jetpack or only on Atomic? My initial guess was that this should only be enabled on Atomic.

Yep, only Atomic for now, I would think. Headstart annotations have not been widely used (if at all) for non-Atomic Jetpack sites.

@andres-blanco
Copy link
Contributor

andres-blanco commented Jul 27, 2022

Closing this since the work done by this PR was split in this other 2:

@github-actions 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 Jul 27, 2022
@sixhours sixhours deleted the add/theme-modal-atomic-support branch February 8, 2023 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE [Feature] Theme Showcase The theme showcase screen in Calypso in Appearance > Themes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Theme Switch Modal: Option to select home page content from demo when switching missing from Atomic
3 participants