Dashboard: add On this day and Achievements toggles to Notifications Extras - #112442
Conversation
Jetpack Cloud Live (direct link)
Automattic for Agencies Live (direct link)
Dashboard Live (dotcom) (direct link)
|
e7e6a8f to
f0120fe
Compare
| } ); | ||
| }, | ||
| } ); |
There was a problem hiding this comment.
[fix here] Issue: Error snackbar doesn't follow the project's "Failed to…" convention and is inconsistent with the existing error message in index.tsx line 85 ('Failed to save subscription settings.').
Error message should be helpful where possible, although it is not always possible. The message should begin with "Failed":
Failed to save {setting name}.
Suggestion:
| } ); | |
| }, | |
| } ); | |
| createErrorNotice( | |
| sprintf( | |
| /* translators: %s is the name of the setting */ __( 'Failed to save %s settings.' ), | |
| __( 'Achievements' ) | |
| ), | |
| { type: 'snackbar' } | |
| ); |
There was a problem hiding this comment.
Fixed in 9b5d579 — error notice now uses the Failed to save %s settings. pattern.
| }, | ||
| } | ||
| ); |
There was a problem hiding this comment.
[fix here] Issue: Same as achievements-card — error snackbar should begin with "Failed" per the project convention and to stay consistent with the existing error in this screen.
Error message should be helpful where possible, although it is not always possible. The message should begin with "Failed":
Failed to save {setting name}.
Suggestion:
| }, | |
| } | |
| ); | |
| createErrorNotice( | |
| sprintf( | |
| /* translators: %s is the name of the setting */ __( 'Failed to save %s settings.' ), | |
| __( 'On this day' ) | |
| ), | |
| { type: 'snackbar' } | |
| ); |
There was a problem hiding this comment.
Fixed in 9b5d579 — error notice now uses the Failed to save %s settings. pattern.
| const { data: notifications } = useSuspenseQuery( | ||
| userPreferenceQuery( 'achievements-global-notifications' ) | ||
| ); |
There was a problem hiding this comment.
[fix here] Issue: useSuspenseQuery will suspend this component, but the route loader at app/router/me.tsx:1013 only prefetches userNotificationsSettingsQuery(). Because userPreferenceQuery isn't prefetched, there'll be a waterfall: the notification settings query resolves first, the component tree starts rendering, then this card suspends a second time to fetch preferences.
Data is prefetched through route loaders where possible
A good strategy is to use a router's
loaderfunction to fetch just enough data to allow a component's layout to be rendered definitively.
Suggestion: Add rawUserPreferencesQuery() (or userPreferenceQuery('achievements-global-notifications')) to the route loader so both fetches run in parallel:
loader: async () => {
await Promise.all( [
queryClient.ensureQueryData( userNotificationsSettingsQuery() ),
queryClient.ensureQueryData( rawUserPreferencesQuery() ),
] );
},There was a problem hiding this comment.
Fixed in 9b5d579 — the Extras route loader now prefetches rawUserPreferencesQuery() in parallel with the notification settings query, so the Achievements card no longer causes a second suspense round-trip.
…tras route loader
|
@Automattic/lego please feel free to review! I'll address in a follow up. |
|
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/34249462 Some locales (Hebrew) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Hi @alshakero, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include this string: Thank you in advance! |
| checked={ !! data.other.timeline.on_this_day } | ||
| disabled={ isMutating } | ||
| label={ __( 'On this day' ) } | ||
| help={ __( 'Reminders about your posts from past years' ) } |
There was a problem hiding this comment.
Nit: Should have a "." at the end.
There was a problem hiding this comment.
Also, these are notifications in the Notification panel correct? The "Achievements" section makes that explicit: "Receive notifications when you unlock new achievements.". Should we be explicit for this one as well?
I guess it's confusing because we have "Extras" in the notifications section but then the section below is "Email from WordPress.com". I wonder why that section isn't in the me/notifications/emails section... Unrelated to this PR. I'll open an issue for that.
There was a problem hiding this comment.
Frankly, I copied this as-is from /me/notifications just to fix the bug. @Copons probably understands this better.
There was a problem hiding this comment.
Nit: Should have a "." at the end.
Fixed in #112464
There was a problem hiding this comment.
@alshakero I think Steven was recommending to make the OTD copy more explicit. Something like this:
- Reminders about your posts from past years.
+ Receive notification reminders about your posts from past years.There was a problem hiding this comment.
Uh oh, my bad. I'll take a nap and check this out. Apparently I'm not functioning well.
There was a problem hiding this comment.
My fault for rambling! Thanks for the updates!
| disabled={ isPending } | ||
| label={ __( 'Achievements' ) } | ||
| help={ __( | ||
| 'Receive notifications when you unlock new achievements. This setting overrides site-level settings.' |
There was a problem hiding this comment.
"This setting overrides site-level settings"? I don't quite understand this. I have site-level Achievements settings?
There was a problem hiding this comment.
I have site-level Achievements settings?
@StevenDufresne yep!
In MSD, they have been moved to /me/notifications/sites. Just open a site and...
There was a problem hiding this comment.
Thinking about it, when the global setting was originally introduced, it contained a link to /me/notifications:
The link was dropped when the setting was moved out of the Achievements screen and into /me/notifications.
Now that we have separate Notifications -> Sites and -> Extras screens, it would make sense to restore the link. I'll look into it. 🙂
There was a problem hiding this comment.
Thanks! Likely pedantic, but site-level threw me off a bit. That felt like something in wp-admin. Maybe it's just the way I think about levels.
Maybe it's easier to read as:
Receive notifications for new achievements. Site-specific achievement notification settings will be ignored.
There was a problem hiding this comment.
Not pedantic at all! This was an RSM project, and we certainly didn't focus on a label copy. 😅
I don't know if "ignore" is correct.
Disabling achievement notifications globally certainly ignores any enabled site-specific notifications, but the opposite is not true. You can keep notifications on globally and disable them per-site.
The problem is that achievements can be about a specific site, but also about the user in general. "Old" achievements were most/all about a specific site, but those we introduced are largely about the user. Hence the reason for adding a global disable.
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~0 bytes added 📈 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~2 bytes added 📈 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~2 bytes added 📈 [gzipped]) DetailsReact 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. |
Fixes phcsdm-PD-p2#comment-5237
Proposed Changes
other.timeline.on_this_dayfield of the/me/notifications/settingsendpoint (the same field the classic notification settings page saves).achievements-global-notificationsuser preference viauserPreferenceQuery/userPreferenceOptimisticMutation(same storage as the classic page, so the two UIs stay in sync).calypso_dashboard_notifications_timeline_settings_updatedwithsetting_name: 'on_this_day', andcalypso_dashboard_notifications_achievements_settings_updated)./me/preferencesmock for existing tests since the Achievements card now suspends on that endpoint.Why are these changes being made?
Testing Instructions
yarn test-client client/dashboard/me/notifications-extras.yarn start-dashboardand go to Notifications → Extras."<name>" settings saved.snackbar should appear, and the state should match the corresponding toggles in the classic notification settings page after a reload of either page.Pre-merge Checklist