-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Create selectors for the Live Preview feature #80017
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~13 bytes removed 📉 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
bb39fae
to
2f4cd9a
Compare
2f4cd9a
to
a66c3fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor! Leave minor feedback but they're not blockers 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but I'd like to encourage you to use Typescript for new files and components introduced in this PR, if possible!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will create a follow up PR 😄
ebea88e
to
058c7fe
Compare
@okmttdhr could you confirm that the |
Always use siteId to get the URL. And use sourceSiteId when it prevents Layout Shift.
Thanks for pointing that out. I addressed it in c204113. |
I'm merging this PR! |
|
||
// Block Theme Previews need the theme installed on Atomic sites. | ||
const isAtomic = isSiteAutomatedTransfer( state, siteId ); | ||
const isThemeInstalledOnAtomicSite = isAtomic && !! getTheme( state, sourceSiteId, themeId ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's open a follow-up PR 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look. Where did you see this error? Maybe on the Block Theme Previews page (= Site Editor)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@taipeicoder One more question, on which theme did you see the error, as an example? Thanks! 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened the issue #80133 to track this 🙂
Related to #79219, #79218, #79775
Proposed Changes
Extract functions added in #79775 into selectors. This allows us to reuse functions between #79219 and #79218.
Testing Instructions
Simple sites
Atomic sites
See if clicking the "Live Preview" button redirects you to the Site Editor (Block Theme Previews).-> I am not yet sure how to patch it to make BTP work (c.f. https://github.com/Automattic/dotcom-forge/issues/3322). We can just check if the "Live Preview" button exists or not.Please see pbxlJb-3Uv-p2#known-issues about unsupported themes.
I don't think we should go through everything in this PR since I just extracted some functions from #79219.
Pre-merge Checklist