Skip to content

Conversation

@taipeicoder
Copy link
Contributor

@taipeicoder taipeicoder commented Oct 24, 2022

Proposed Changes

This PR updates the style preview UI in the design preview component. Previously, we couldn't update the style preview UI without updating the package @wordpress/edit-site, which is a complicated ordeal (see p1666235331775949-slack-C03DY3RJ5 and pbxlJb-2gk-p2#comment-1952). Also, see the abandoned PR that attempts to do so: #69272.

As an alternative solution, instead of updating @wordpress/edit-site, we make a local copy of the component <StylesPreview /> instead of directly importing it from @wordpress/edit-site. The downside of this approach is that we no longer benefit from the abstraction provided by the package, by the upside is that we can now update the style preview UI without needing to bump @wordpress/edit-site and its dependencies across Calypso. This approach can also potentially prevent us from having to update @wordpress/edit-site each time the style preview UI changes.

Testing Instructions

  • Head to the design picker setup/designSetup?siteSlug=${site_slug}
  • Open any design with style variation.
  • Ensure that the style preview UI is updated to match the one in the site editor (wordpress.com/site-editor/${site_slug})

Pre-merge Checklist

@github-actions
Copy link

github-actions bot commented Oct 24, 2022

@matticbot
Copy link
Contributor

matticbot commented Oct 24, 2022

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

Async-loaded Components (~348 bytes added 📈 [gzipped])

Details
name                                  parsed_size           gzip_size
async-load-automattic-design-preview      +1469 B  (+0.2%)     +348 B  (+0.1%)

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.

@taipeicoder taipeicoder self-assigned this Oct 24, 2022
@taipeicoder taipeicoder added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Oct 24, 2022
@taipeicoder taipeicoder requested a review from a team October 24, 2022 09:26
@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 Oct 24, 2022
@arthur791004
Copy link
Contributor

the upside is that we can now update the style preview UI without needing to bump @wordpress/edit-site and its dependencies across Calypso.

I think it's still case by case. If issues are introduced from any of useSetting, useStyle, or useGlobalStylesOutput, we still need to bump @wordpress/edit-site 😅

As a result, I'd prefer to use the same workaround of #69318 again when we encounter a similar issue next time. What do you think?

@taipeicoder
Copy link
Contributor Author

the upside is that we can now update the style preview UI without needing to bump @wordpress/edit-site and its dependencies across Calypso.

I think it's still case by case. If issues are introduced from any of useSetting, useStyle, or useGlobalStylesOutput, we still need to bump @wordpress/edit-site 😅

As a result, I'd prefer to use the same workaround of #69318 again when we encounter a similar issue next time. What do you think?

Agreed! Which is why I wrote - "This approach can also potentially prevent us from having to update @wordpress/edit-site each time the style preview UI changes." It's definitely not a cure-all solution, but it gives us more leeway.

This approach mainly addresses these feedbacks pdtkmj-Ad-p2#comment-1007 and p1666215740390499-slack-CRWCHQGUB which updates the style preview hover presentation.

Before After
Screen Shot 2022-10-25 at 9 42 54 AM Screen Shot 2022-10-25 at 9 43 49 AM

Unfortunately, we couldn't achieve this UI update with #69318 🥲

@arthur791004
Copy link
Contributor

This approach mainly addresses these feedbacks pdtkmj-Ad-p2#comment-1007 and p1666215740390499-slack-CRWCHQGUB which updates the style preview hover presentation.

Oh yes, that's a fair point 🙃 Let's go with this way first and see whether there is a better solution later. For example, make global-styles an independent package like @wordpress/global-styles or fork a sub-package in the mono-repo to wp-calypso

Copy link
Contributor

@arthur791004 arthur791004 left a comment

Choose a reason for hiding this comment

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

Work nicely 👍

Minor feedback is to make a new package for it and we could keep @automattic/design-preview simpler. But it's optional 🙂

Copy link
Contributor

@fushar fushar left a comment

Choose a reason for hiding this comment

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

Works nicely 😄

As now we have a local copy of the component, we might be able the fix bug in this local copy as described here: pbxlJb-2ow-p2 (in the future).

@taipeicoder
Copy link
Contributor Author

Works nicely 😄

As now we have a local copy of the component, we might be able the fix bug in this local copy as described here: pbxlJb-2ow-p2 (in the future).

That's a good point! I'm up for looking into that, and make it a new package as @arthur791004 suggested, if bandwidth allows 🙂

@taipeicoder taipeicoder merged commit 8f62243 into trunk Oct 25, 2022
@taipeicoder taipeicoder deleted the add/design-preview-style-variation-preview branch October 25, 2022 09:13
@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 Oct 25, 2022
@taipeicoder taipeicoder mentioned this pull request Oct 25, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants