Fix UPE styles extraction in the Site Editor #10433
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +612 B (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
danielmx-dev
commented
Feb 24, 2025
| @@ -50,7 +50,7 @@ export const appearanceSelectors = { | |||
| pmmeRelativeTextSizeSelector: '.wc_payment_method > label', | |||
| }, | |||
| blocksCheckout: { | |||
| appendTarget: '#contact-fields', | |||
| appendTarget: '.wc-block-checkout__contact-fields', | |||
There was a problem hiding this comment.
I had to change since the original selector was not present in edit mode.
danielmx-dev
commented
Feb 24, 2025
Comment on lines
+48
to
+55
| watchPathIgnorePatterns: [ | ||
| '/node_modules/', | ||
| '/vendor/', | ||
| '<rootDir>/.*/build/', | ||
| '<rootDir>/.*/build-module/', | ||
| '<rootDir>/docker/', | ||
| '<rootDir>/tests/e2e', | ||
| ], |
There was a problem hiding this comment.
This is slightly unrelated to the PR but I'm always having issues when trying to run npm run test:watch due to the large number of files. Please let me know if I should revert this or put it in a separate PR.
danielmx-dev
requested review from
a team and
frosso
and removed request for
a team
frosso
reviewed
Feb 27, 2025
There was a problem hiding this comment.
I tested the changes with Chrome, Safari, Firefox, and they seem to behave consistently, so I think your approach is good 👍
In develop on the "preview" pages, I see the loading box match the theme's color scheme if a dark theme is used (in this case, with TT5):
But with these changes, I see the light color scheme for the preview:
Is that something that can be improved?
| </Elements> | ||
| </LoadableBlock> | ||
| <div ref={ containerRef }> | ||
| <LoadableBlock isLoading={ ! appearance } numLines={ 3 }> |
There was a problem hiding this comment.
I suppose that it's a micro-optimization now, but what do you think of changing this line to
Suggested change
| <LoadableBlock isLoading={ ! appearance } numLines={ 3 }> | |
| <LoadableBlock isLoading={ ! appearance || !stripeForUPE } numLines={ 3 }> |
and removing the if ( ! stripeForUPE ) check a few lines above?
I'm thinking that with your changes, the generateUPEAppearance() function won't be executed until stripeForUPE has been provided.
But if instead we move the conditional on stripeForUPE below, we can load Stripe and generate the UPE appearance concurrently.
| /> | ||
| </Elements> | ||
| </LoadableBlock> | ||
| <div ref={ containerRef }> |
There was a problem hiding this comment.
(minor and optional, up to discussion) I have some (minor) concerns about wrapping the whole thing with this div.
I understand that it's necessary to get a reference for the getFontRulesFromPage/getAppearance functions.
I guess my concern lies with the possibility of breaking some CSS selectors 🤷
But what do you think about wrapping everything in a Fragment, and mounting this div just as an empty element somewhere within that fragment?
I'm thinking of something like this:
return (
<>
<LoadableBlock ...>
<Elements ... />
</LoadableBlock>
<div ref={ containerRef }></div>
</>
);Again, optional an up for discussion!
frosso
approved these changes
Feb 27, 2025
There was a problem hiding this comment.
Thank you for connecting earlier @danielmx-dev !
In develop on the "preview" pages, I see the loading box match the theme's color scheme if a dark theme is used (in this case, with TT5)
But with these changes, I see the light color scheme for the preview:
I think we were able to pinpoint this to a skill issue on my side: it looks like I was probably using "light" inputs, instead of the dark ones 🤦
I'm good with your changes, since the other comments are pretty much optional 👍
Thanks again! 
frosso
approved these changes
Feb 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #10335
Changes proposed in this Pull Request
In order to prevent from bleeding into the page preview, the site editor embeds the contents into an iframe, however, react still runs in the context of the host page.
The code that extracts the style also runs within the context of the host page, so any reference to the global window and host document will refer to this host page, instead of the elements that are contained within the page preview, this means that none of the style extraction will be performed as expected.
In this PR, we'll make sure that we use the correct document and window objects when performing the style extraction process.
Testing instructions
Delete UPE appearance transients. Make sure you don't have any other tabs/windows using checkout to prevent the styles from being computed before we finish our testing.In develop
The inputs in the preview are displayed using a dark theme (even if we are using a light theme)
If you open the actual checkout, the inputs will be dark:
Current branch
The inputs will appear with a styling that is more similar to the rest of checkout
If you open the actual checkout page, the styles should be consistent with the rest of the page
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge