-
Notifications
You must be signed in to change notification settings - Fork 2k
Reader onboarding: update profile complete criteria and Gravatar status #103602
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
@@ -13,7 +13,7 @@ import { isProfileComplete } from './utils'; | |||
|
|||
function dispatchNotice( dispatch: Store[ 'dispatch' ] ) { | |||
dispatch( | |||
successNotice( translate( 'Profile complete!' ), { | |||
successNotice( translate( 'Settings saved!' ), { |
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 changed the wording here to reflect that your profile may not be 100% complete.
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
@@ -40,11 +40,7 @@ export const dispatchProfileCompleteNotice = ( store: Store, action: UnknownActi | |||
...state.userSettings.settings, | |||
...( typeof action.settingValues === 'object' ? action.settingValues : {} ), | |||
}; | |||
const gravatarDetails = { | |||
...state.gravatarStatus, | |||
gravatarDetails: { has_gravatar: !! state.gravatarStatus.tempImage }, |
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 may not have a tempImage
, and we don't want to overwrite true
if the avatar has been added.
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~378 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~241262 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. Async-loaded Components (~1017 bytes added 📈 [gzipped])
React 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. |
Just tested this. @allilevine I'm now seeing a double notification anytime I save (after coming from the onboarding flow): |
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.
Pull Request Overview
This PR relaxes the reader onboarding profile-complete check to succeed if any profile field or a Gravatar exists, and ensures a success notice is shown when the avatar is updated.
- Change
isProfileComplete
to use||
instead of&&
, so filling a single field or uploading an avatar counts as complete. - Update the onboarding handler to listen for
GRAVATAR_DETAILS_RECEIVE
and adjust notice text. - Add
receiveGravatarDetails
dispatch in the Gravatar editor component and update tests.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
client/state/reader/onboarding/utils.ts | Switched profile-complete criteria from all fields (&& ) to any (` |
client/state/reader/onboarding/handlers.ts | Updated action type to GRAVATAR_DETAILS_RECEIVE and notice message. |
client/state/data-layer/wpcom/gravatar-upload/index.js | Registered dispatchProfileCompleteNotice on GRAVATAR_DETAILS_RECEIVE . |
client/blocks/edit-gravatar/index.jsx | Added receiveGravatarDetails prop dispatch on avatar update. |
client/blocks/edit-gravatar/test/index.jsx | Updated test to expect receiveGravatarDetails call. |
Comments suppressed due to low confidence (2)
client/state/reader/onboarding/handlers.ts:14
- [nitpick] The notice ID
reader-profile-complete
no longer matches the updated messageProfile updated!
; consider renaming the ID to something likereader-profile-updated
for clarity and consistency.
id: 'reader-profile-complete',
client/state/reader/onboarding/utils.ts:8
- The new logic in
isProfileComplete
(allowing any single field orhas_gravatar
) isn't covered by existing unit tests; consider adding tests for cases where only one profile field or only the gravatar flag is set.
settings.last_name ||
@davemart-in That's expected, unfortunately -- we're adding this extra notice when coming from the onboarding flow, but we also always show a notice when you save your user settings. It's triggered after our notice, so it's difficult to remove. I can try a couple more things but I don't want to prevent that notice from showing up normally. |
@davemart-in I had an idea and added a check for the url param in the settings data layer. It's mixing concerns but it'll prevent the second notice. |
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.
Tested again. Works as expected this time. Thanks!
Co-authored-by: Copilot <[email protected]>
5d309e9
to
e6e5df7
Compare
Fixes https://linear.app/a8c/issue/CML-497/no-link-back-to-reader
Proposed Changes
has_gravatar
totrue
when avatar is updated, and show a success notice.ref=reader-onboarding
.Why are these changes being made?
Testing Instructions
Note that the success notice will show even if you delete your Gravatar image, because that's also a change, but that's unlikely to be the case for someone in Reader Onboarding.
Pre-merge Checklist