-
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
Launchpad: Add domain ssl processing notification #69317
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~75 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. 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. |
UPDATE: Issue above with free .wordpress.com domains showing the message has been resolved.
|
5c53559
to
db7137c
Compare
Thanks for the patience with this @edanzer. I pushed a fix with tests to cover the null case |
LGTM.
|
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.
Changes look good and this tests well.
Testing
✔️ Notification is showing when using a pain domain with a personal plan while domain is pending
✔️ Notification is not showing when using a free domain
Regarding the notification - are we able to add the circle green check icon and corresponding text offset as shown in the similar design here ? |
client/landing/stepper/declarative-flow/internals/steps-repository/launchpad/sidebar.tsx
Outdated
Show resolved
Hide resolved
Changes look good except for the green checkmark @Addison-Stavlo noted above. Testing[x] Notification shows with new custom domain |
I wasn't certain if we wanted to add the icon as well, but I'm happy to do so in a follow-up |
Changes look good. Retested this after recent changes, and tests as expected. The issue I noted above with free .wordpress.com domains showing the new message is resolved. I also ran the unit tests, and they are all passing. |
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7596273 Thank you @jeyip for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Proposed Changes
Screenshots
Testing Instructions
Pre-merge Checklist
Related to #68967