-
Notifications
You must be signed in to change notification settings - Fork 2k
Notification Settings: Enable to configure notification settings by site #106080
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
Notification Settings: Enable to configure notification settings by site #106080
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
…n-settings-by-site
Squash with rename
Remove external link
…n-settings-by-site
|
@lucasmendes-design Do you think we should show a modal warning the users if they click on the "Apply to all sites" without reading it? I got myself doing it, thinking it was a regular save button. |
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.
@lucasmendes-design do we have design for this page? In the current form, the UI and interactions in this page looks a bit off compared to the rest of HD pages 🤔
| ...userNotificationsSettingsMutation(), | ||
| meta: { | ||
| snackbar: { | ||
| success: __( 'Settings saved successfully.' ), |
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.
We now have guidelines re: snackbar copy, please take a look 😄
client/dashboard/me/notifications-sites/device-settings/index.tsx
Outdated
Show resolved
Hide resolved
What do you mean? @fushar The latest is here: |
|
The previous iteration is here: But we needed to move away from this since @gabrielcaires was giving some feedback regarding the UI and how our current components work. |
Oops, I missed that, sorry! My feedback is that for a12s who have hundreds of sites, that page will:
|
d0933a6 to
70f69bc
Compare
It's fair feedback. For users who have multiple sites, they can search for a website using the search input. The pagination, though, is a good solution to make this page faster. Is it possible to add? Also, this scenario you just described is very rare for real users. |
@lucasmendes-design and @fushar We can add a skeleton to be displayed while the list of sites is still loading; it is easy to implement. Regarding the pagination, I suggest moving it to the next iteration, considering it is an edge case that will require building backend changes to support pagination and search (the search is happening directly on the frontend). My concern is that the project is already ahead of the expected target date, so it is better to increase the scope only when we have a critical case. |
@lucasmendes-design Modal applied, thanks
|
|
I updated the PR with an initial loading stat. We can improve it if we want further PRs (just to not block this one) I used the same spinner we are using on the /site table. CleanShot.2025-10-01.at.16.55.33.mp4 |
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.
We can add a skeleton to be displayed while the list of sites is still loading; it is easy to implement.
Regarding the pagination, I suggest moving it to the next iteration, considering it is an edge case that will require building backend changes to support pagination and search (the search is happening directly on the frontend).
Fair points. Agree to both points.
The code looks good. I left a small comment. 👍
.eslintrc.js
Outdated
| '__experimentalDivider', | ||
| '__experimentalGrid', | ||
| '__experimentalHStack', | ||
| '__experimentalBadge', |
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 don't think this is actually being used.
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.
Removed, thanks!
Do we use them across the product? I thought we would remove it. |
| <CardBody> | ||
| <HStack> | ||
| { header } | ||
| <Button |
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.
This will need to be accessible.
Some issues:
- Button needs an aria-label.
- Need to communicate aria-expanded state.
- Need to connect the button to the content.
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.
Good point! I will do it in the next PR because this one is already too big.
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.
Sounds good! Maybe consider moving this to the component folder as well. I know working on the performance tab, I was looking for something with this functionality.
@lucasmendes-design Yes, it is used across multiple components on the new hosting dashboard screen. It may be difficult to see because we have an aggressive cache strategy, but you can view it more clearly if you delete this record from your
|



Close CML-849
Proposed Changes
Testing Instructions
[I recommend testing it running in your local env and using a non-A8C to not mess with your notification settings)
/v2/me/notifications/sitesScreenshots
Regular (First site doesnt have site name)
No site found
Opened site