-
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
Add Jetpack connect screen loading screens and minor UI update #78665
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~13 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~2226 bytes added 📈 [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. |
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 work! LGTM and tested well 👍 🚀
@rjchow Could you please confirm the "jpVersion" in your URL parameters? I guess @ilyasfoo also encountered that issue, but I was not able to reproduce it. See: #78615 |
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.
Opening up a modal to show the loading is a creative solution, good thinking @chihsuan!
One thing however, I don't know if there are any instances where the connect would error, and now it might not be shown since modal is covering it. Is that a possibility we should consider?
Also, it seems no actual progress is being passed to WooLoader
. I may not understand the loading process 100%, but should we use actual loading instead of timer and intervals?
Anyway, this PR is tested well. Everything else can be addressed as a follow-up if necessary! 🚢
Could you please confirm the "jpVersion" in your URL parameters? I guess jpVersion param is missing.
@chihsuan This time around I wasn't able to reproduce it. My URL has jp_version=12.2.1
.
Edit: I was able to reproduce it again by deactivating Jetpack plugin before going through the core profiler. Probably not something we should address in this PR though.
![image](https://private-user-images.githubusercontent.com/3747241/249520887-d5a2c34c-0c74-4a94-9343-d287146c27f2.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk0MTg2ODksIm5iZiI6MTczOTQxODM4OSwicGF0aCI6Ii8zNzQ3MjQxLzI0OTUyMDg4Ny1kNWEyYzM0Yy0wYzc0LTRhOTQtOTM0My1kMjg3MTQ2YzI3ZjIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxMyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTNUMDM0NjI5WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZjMxOGEzMjU5MDhiMTQ0Yzg0YjcyNzYzM2MyOGIxMWU2NzJkZmNiYTZhZjA1YzkwNGRiOWU0NzRhYmUwYTRiZSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.WT0H0S7eZiQ9aXsxuiwD66QdtdmexCqpT0prCEkPrlw)
Thanks for the review! @ilyasfoo
Actually, error will be shown because we only render the modal when
Yes, I agree with you! It's not the best approach for now. I think we should rethink the design of the component. We can work on that when creating a reusable component.
Thanks, I was able to reproduce it this time. I think JP connect package doesn't set the |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/8069144 Thank you @chihsuan for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Related to #78267
Proposed Changes
Screen.Recording.2023-06-27.at.14.28.28.mov
Testing Instructions
Logged-in users
Connect your account
buttonNew users
Administrator
role to itCreate an account
buttonPre-merge Checklist