-
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
Jetpack Pro Dashboard: implement monitor notification settings modal #71373
Jetpack Pro Dashboard: implement monitor notification settings modal #71373
Conversation
a3e020a
to
99c8dee
Compare
...t/jetpack-cloud/sections/agency-dashboard/downtime-monitoring/notifcation-settings/index.tsx
Outdated
Show resolved
Hide resolved
...t/jetpack-cloud/sections/agency-dashboard/downtime-monitoring/notifcation-settings/index.tsx
Outdated
Show resolved
Hide resolved
Is the |
@DustinHartzler - Thanks for your review on this PR. We will handle the API integration for |
Thank you @yashwin! I just left two responses to the questions; otherwise, this 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.
Lgtm!
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~60 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~2044 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. |
5ba3e76
to
19ff752
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.
Awesome work, @yashwin! This PR looks and works great on all device sizes! I've left some minor feedback.
I also have one UX question: how easy would it be to allow Space
as a delimiter? I think it's a pretty common UX pattern, and if we can easily integrate it, that would be great.
client/jetpack-cloud/sections/agency-dashboard/sites-overview/utils.ts
Outdated
Show resolved
Hide resolved
.../jetpack-cloud/sections/agency-dashboard/downtime-monitoring/notification-settings/index.tsx
Outdated
Show resolved
Hide resolved
.../jetpack-cloud/sections/agency-dashboard/downtime-monitoring/notification-settings/index.tsx
Show resolved
Hide resolved
.../jetpack-cloud/sections/agency-dashboard/downtime-monitoring/notification-settings/index.tsx
Outdated
Show resolved
Hide resolved
.../jetpack-cloud/sections/agency-dashboard/downtime-monitoring/notification-settings/index.tsx
Outdated
Show resolved
Hide resolved
...ack-cloud/sections/agency-dashboard/downtime-monitoring/toggle-activate-monitoring/index.tsx
Show resolved
Hide resolved
...ack-cloud/sections/agency-dashboard/downtime-monitoring/toggle-activate-monitoring/index.tsx
Outdated
Show resolved
Hide resolved
Thanks for your review, @vitozev. I have addressed all your comments.
Yes, agree. I have added this support too. |
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.
LGTM!
234cf01
to
2b7063e
Compare
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7688748 Thank you @yashwin 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
This PR implements monitor notification settings modal in the Jetpack Pro Dashboard.
Testing Instructions
Prerequisites
Since these changes are made specifically for agencies, you must set yourself(partner) as an agency - 2c49b-pb. Make sure to switch it back to the previous type.
Instructions
git checkout add/implement-monitor-notification-settings-modal
andyarn start-jetpack-cloud
Mobile view
Tablet view
Pre-merge Checklist
Related to 1203448324265423-as-1203507036638783