-
Notifications
You must be signed in to change notification settings - Fork 111
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
Polish social media flow, other QA [C-3418] #6887
Conversation
|
||
if (signUpState.handle.value) { | ||
// Already have handle or it needs review | ||
allowedRoutes.push(SignUpPath.reviewHandle) | ||
allowedRoutes.push(SignUpPath.finishProfile) | ||
|
||
if (signUpState.name.value) { | ||
if (signUpState.finishedPhase1) { |
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.
introduced the concept of "phase 1" which is only complete once you finish the "finish profile" page. otherwise, if you connected social media, you were forced onto the genres page because the display name is set from social media, but we actually want you to see the "finish profile" page first.
Preview this change https://demo.audius.co/nkang--socials-refine |
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.
Really well thought out with all the complexities of the flows. I think the main thing id like to see improved is a common hook so it hides a bit of the logic and consolidates it, so it feels like we are adding on a feature to the pages rather than the pages having to implement the same thing twice. lmk if its more complicated than that though
{messages.connectingSocialMedia}… | ||
</Text> | ||
<Box mb='2xl'> | ||
<LoadingSpinner className={styles.spinner} /> |
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.
css prop can be css={{g: {path: {stroke: theme.color.neutral['n-800']}}
(i forget the exact theme variable)
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.
Can't put the css prop on this since it comes from web :/
</Hint> | ||
)} | ||
{(errorMessage) => | ||
dirty ? ( |
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.
nice check on dirty here... so actually what is the situation, is the email carried over from sign-in page?
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.
Before I added this, if you selected one of the social media options, the form would validate (because it's initially autofocused) and say "Please enter an email"
label={messages.handleLabel} | ||
value={ | ||
<Flex alignItems='center' gap='xs'> | ||
@{handleField.value} |
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.
should we create a "handle" component that can have "isVerified" as a flag? just thinking we will prob need this in like 3 or 4 places
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.
yeah I think if we use it in more places we should, but I didn't see any other places where we needed it
@@ -127,7 +135,7 @@ export const FinishProfilePage = () => { | |||
centered | |||
buttonProps={{ disabled: !isValid }} | |||
postfix={ | |||
isMobile ? null : ( | |||
isMobile || isSocialConnected ? null : ( |
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.
nice call ^ do we remove the "step 2 of 2" also in this case?
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.
ooh good question... we should remove it if you connected social media on the email page
|
||
const initialValues = { | ||
email: existingEmailValue.value ?? '' | ||
} | ||
|
||
useEffect(() => { |
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.
can this useEffect + state + handlers be part of a hook that we can use in CreateEmailPage and PickHandlePage?
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.
Hmm I think that would be over engineering it, and the actions for resetting the state are different for the two pages (in this page, we call resetSignOn
, on the other page, we call unsetSocialMedia
)
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 think I can make this work though, I'll add it in
Preview this change https://demo.audius.co/nkang--socials-refine |
…ment-router * origin/main: Commit package-lock changes (#6895) [PAY-2221] Purchase content through payment router (#6880) Re-remove copy . . from discovery dev dockerfile (#6889) [C-3456] Improve mobile user-list loading (#6845) Address mobile icon issues (#6893) Polish social media flow, other QA [C-3418] (#6887) relay: fix error on no metadata (#6883) Bump version to 0.5.34 [Harmony] TextInput - show red error if input too long (#6886)
[3c4193e] Fix svgo viewBox defaults (#6901) Dylan Jeffers [3e414de] Improve mobile clean commands (#6896) Sebastian Klingler [974ca03] Fix rewards + plays indexing (#6898) Reed [7bf5e6d] [PAY-2245] Fix purchase radio buttons on android (#6897) Reed [0559f41] Commit package-lock changes (#6895) Sebastian Klingler [976b787] [PAY-2221] Purchase content through payment router (#6880) Reed [5c56fcb] Re-remove copy . . from discovery dev dockerfile (#6889) Raymond Jacobson [f38b6b3] [C-3456] Improve mobile user-list loading (#6845) Dylan Jeffers [51c4273] Address mobile icon issues (#6893) Dylan Jeffers [fafdec4] Polish social media flow, other QA [C-3418] (#6887) nicoback2 [9bced7d] relay: fix error on no metadata (#6883) alecsavvy [056be80] Bump version to 0.5.34 audius-infra [9e86ab2] [Harmony] TextInput - show red error if input too long (#6886) nicoback2 [f9de2f3] [INF-557] Convert libs to esm so vite HMR works (#6882) Sebastian Klingler [d8f2443] [C-3410] Native password page + Common rollup changes (#6874) JD Francis [503245a] Remove extraneous icon imports (#6885) Dylan Jeffers [19502f1] [C-3423] Add Avatar component to native harmony (#6884) Kyle Shanks [93b0280] feat: Add text align to Text component props (#6856) JD Francis [71f093e] [C-3431] Add universal harmony icons (#6877) Dylan Jeffers [7df0fda] [Sign Up Redesign] Fix sign up allowed routes [C-3488] (#6879) nicoback2 [dc442e0] Bump version to 0.5.33 audius-infra [888789f] PROTO-1364: fix relay heavy load (#6878) alecsavvy [67554f1] [PAY-2197] Check Redis cache for transactions (#6812) Marcus Pasell [bc26c8b] [PAY-2194] Update summary tables (#6871) Saliou Diallo [28b90ad] [C-3458] Sign Up QA round 2 (#6851) Dylan Jeffers [93f97b7] [C-3479] 404 on invalid collection url (#6875) Sebastian Klingler [6ccf322] [C-3332] Fix playlist edit after create (#6873) Sebastian Klingler [6da7e8c] [C-2959] Fix creating playlist from track (#6868) Sebastian Klingler [498c071] [C-3480] Fix null description in Edit Playlist (#6870) Andrew Mendelsohn [8909f03] fix openresty version (#6869) Steve Perkins [977dd55] [Mobile] [C-3413] Fix verified sign up on mobile via Twitter/IG (#6855) nicoback2 [1cbc11c] PROTO-1474: Discovery uses db tempalte for integration tests (#6853) Steve Perkins [165211e] Bump version to 0.5.32 audius-infra [67d6f27] [C-3457] Simplify harmony-native theme (#6859) Dylan Jeffers [3a390a7] [PROTO-1478] Add finalPOABlock support to discovery relay (#6861) Raymond Jacobson [e57b781] Fix discovery lint err (#6867) Michelle Brier [bee4820] Reindex corrupted discovery indexes (#6865) Raymond Jacobson [9946c48] Payment router instructions in @audius/spl (#6862) Reed [37b8a6a] Move node-ui to audius-d (#6823) endline [cd431cb] Fix typo in libs solana decimals constants (#6863) Reed [92dc9bf] Add payment router addresses (#6860) Raymond Jacobson [f59952c] [PROTO-1477] Free up mediorum db space and remove unused code (#6852) Theo Ilie [f998d54] [C-3408] Native Create Account Page (#6838) JD Francis [e4f9c41] Update solana-test-validator container build to allow deployment of payment router (#6848) Randy Schott [a62e5af] [C-3467] Fix python install in mobile ci (#6857) Sebastian Klingler [bc4d099] [Mobile] Fix profile + cover pic not uploading upon social sign up [C-3466] (#6850) nicoback2 [c6dfb19] re-add docker copy (#6854) Steve Perkins [0bf73ab] [PAY-2237] Allow for purchaser_user_id in memo (#6849) Raymond Jacobson [d9959c5] Bump version to 0.5.31 audius-infra [de5c6d0] Move es-indexer to separate pacakge (#6841) Steve Perkins [16e6bc7] [C-3459] Improve sign-up responsiveness (#6844) Dylan Jeffers [f72fdeb] Periodically check for and correct trusted notifier discrepancies in DN (#6832) Michelle Brier [69a08ab] Remove track_segments data, default to [] (#6835) Steve Perkins [7705e1c] [C-3385] Harmony icon component tweaks (#6837) JD Francis [81202f7] [PAY-2236] USDC Balance in mobile nav updates after purchase (#6842) Reed [a9b35ad] Fix mobile premium track remix visibility (#6843) Saliou Diallo [942fcb8] Return remixes for premium tracks (#6840) Saliou Diallo [7089475] Fix remixes for usdc purchase tracks (#6839) Saliou Diallo
Description
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.