-
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
Move WebPreview to calypso-ui package #34337
Conversation
Switches translate functions to use wordpress/i18n and moves recorded events and redux state changes to parent component.
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~1854 bytes removed 📉 [gzipped])
Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded. App Entrypoints (~97568 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~363261 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. Async-loaded Components (~259576 bytes removed 📉 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
…n the Popover component.
It looks like my changes fixed the command line tests, but we still have one failure in the desktop tests around image/svg handling. https://circleci.com/gh/Automattic/wp-desktop/41728 I wonder if we need something like https://github.com/Automattic/wp-calypso/blob/master/packages/calypso-build/jest.config.js and/or https://github.com/Automattic/wp-calypso/blob/master/packages/calypso-build/webpack/file-loader.js for building the ui package? |
It looks like the |
Hi @joshuatf 👋 Thanks you for working on this component: it looks like a huge task 🙂 The reason why the Desktop build fails is that the Desktop Server imports the Themes section (which is server-side-rendered) which in turns imports the Web Preview component, and the That would be an easy patch, but the error reveals a more serious issue: async loading of the Can you look into that issue? None of the Web Preview code should be loaded until some preview is actually displayed. However, the current I think the best way to proceed is to start merging non-controversial parts of this big patch as separate PRs. For example, moving This diff is really big, I see many weird and suspicious things when skimming through it, but I'm unable to do a comprehensive review of a patch of this size. Some tips:
|
This issue has been marked as stale and will be closed in seven days. This happened because:
You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation. |
Changes proposed in this Pull Request
WebPreview
component and necessary dependencies to thecalypso-ui
package.WebPreview
component and separates device logic.AsyncLoad
component tocalypso-ui
.SpinnerLine
component tocalypso-ui
.Thoughts
translate
prop added here or another error. Any feedback here appreciated!WebPreview
component. While some of this logic could benefit from being shared across projects, there are many dependencies tied up in Calypso.hasTouch()
method was manually copied over to avoid creating a newcalypso-lib
package.Testing instructions
WebPreview
component and make sure they continue to work as expected with correct buttons/content appearing based on props.seo
).SpinnerLine
continues to work across the site.npm link
the calypso-ui package, bump the version, and test that you can import theWebPreview
component in another project.Fixes #34295