Skip to content
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

[C-3403] Fix sign-up e2e #6815

Merged
merged 4 commits into from
Dec 1, 2023
Merged

[C-3403] Fix sign-up e2e #6815

merged 4 commits into from
Dec 1, 2023

Conversation

dylanjeffers
Copy link
Contributor

Description

Fixes sign-up e2e

  • Improves useFeatureFlag (@schottra lets chat)
  • Improves FollowButton (@amendelsohn was tricky here so let's chat)

How Has This Been Tested?

signUp.cy.ts should pass. Additionally going to tackle the mobile flow tomorrow in this pr

@dylanjeffers dylanjeffers requested review from a team and sliptype and removed request for a team November 30, 2023 03:28
Copy link
Contributor

@sliptype sliptype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

}
getOverride()
})

return {
isLoaded: configLoaded,
isLoaded: configLoaded && isLocallyEnabled !== undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I like this! Maybe a comment would be good to clarify the null vs undefined state

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i really dont love how it can be undefined | null | false | true... probably a status boolean is ideal. @schottra do you have any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally in favor of null being an explicitly set empty value (vs. undefined meaning our code never touched it and it's empty).

I do think it's a little cryptic to derive "has localStorage been read" from the value of a variable we attempted to read out of local storage. An explicit boolean indicating that we finished fetching the item from localStorage might be better.

const [isLocallyOverridden, setIsLocallyOverridden] = useState<Maybe<boolean>>()
const [hasFetchedLocalStorage, setHasFetchedLocalStorage] = useState(false)

useEffectOnce(() => {
  const getOverride = async () => {
    const override = await localStorage.getItem(overrideKey)
    if (override === 'enabled') {
      setIsLocallyOverridden(true)
    } else if (override === 'disabled' { 
      setIsLocallyOverridden(false)
    }
    setHasFetchedLocalStorage(true)
  }
})

return {
  isLoaded: configLoaded && hasFetchedLocalStorage
  isEnabled: isLocallyOverridden ?? isEnabled
}

Also, wanted to point out that the variable name and the setter don't match and by convention they probably should to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like this alot, will go with it

@@ -69,8 +67,7 @@ describe('Sign Up', () => {
assertOnSignUpPage()
})

// TODO: IN TEST JAIL [C-3403] turn back on when full account creation flow is set up
it.skip('should create an account', () => {
it('should create an account', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's goooo

const [, setIsWelcomeModalOpen] = useModalState('Welcome')
const [currentGenre, setCurrentGenre] = useState('Featured')
const dispatch = useDispatch()
const navigate = useNavigateToPage()
const { color } = useTheme()
const headerContainerRef = useRef<HTMLDivElement | null>(null)

console.log({ currentGenre })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loggy boi

@@ -166,6 +175,10 @@ export const SelectArtistsPage = () => {
css={{ minHeight: 500 }}
direction='column'
>
<HiddenLegend>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its for a11y, where the checklist fieldset has a label, which comes from a vs aria-label

}

return undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find myself wondering why this was here in the first place? Seems like it was meant to make sure the effect got undefined as a return value, but it doesn't seem like this return value was ever used.

}
getOverride()
})

return {
isLoaded: configLoaded,
isLoaded: configLoaded && isLocallyEnabled !== undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally in favor of null being an explicitly set empty value (vs. undefined meaning our code never touched it and it's empty).

I do think it's a little cryptic to derive "has localStorage been read" from the value of a variable we attempted to read out of local storage. An explicit boolean indicating that we finished fetching the item from localStorage might be better.

const [isLocallyOverridden, setIsLocallyOverridden] = useState<Maybe<boolean>>()
const [hasFetchedLocalStorage, setHasFetchedLocalStorage] = useState(false)

useEffectOnce(() => {
  const getOverride = async () => {
    const override = await localStorage.getItem(overrideKey)
    if (override === 'enabled') {
      setIsLocallyOverridden(true)
    } else if (override === 'disabled' { 
      setIsLocallyOverridden(false)
    }
    setHasFetchedLocalStorage(true)
  }
})

return {
  isLoaded: configLoaded && hasFetchedLocalStorage
  isEnabled: isLocallyOverridden ?? isEnabled
}

Also, wanted to point out that the variable name and the setter don't match and by convention they probably should to avoid confusion.

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/dj-3403-fix-sign-up-e2e

@dylanjeffers dylanjeffers merged commit d168c29 into main Dec 1, 2023
3 of 5 checks passed
@dylanjeffers dylanjeffers deleted the dj-3403-fix-sign-up-e2e branch December 1, 2023 03:48
dylanjeffers added a commit that referenced this pull request Dec 2, 2023
audius-infra pushed a commit that referenced this pull request Dec 2, 2023
[97ee16b] Fix routing for legacy sign-on (#6834) Dylan Jeffers
[c5f150f] [C-3374] Fix play/pause on trending mobile (#6833) Dylan Jeffers
[3612090] [C-3417] Cover photos default to blurred profile photo if unset (#6811) Andrew Mendelsohn
[fb74614] [PROTO-1458] Expose postgres upgrade logs in mediorum (#6830) Theo Ilie
[6deaeab] [C-2591] Fix Follow All button on mobile (#6831) Andrew Mendelsohn
[d952d16] [C-3453] Replace edit collection form with new fields (#6829) Andrew Mendelsohn
[3ed1747] Make mobile buy buttons stick to bottom (#6827) Reed
[4d02605] [PAY-2209] Use single modal for purchase and transfer (#6810) Reed
[f84f24e] [PAY-2227] Set min-width for NavPopupMenu (#6826) Reed
[17ca613] Auto-hide scheduled releases and index as utc (#6824) Isaac Solo
[3d8fbaa] [C-3421] Add email-in-use hint (#6825) Dylan Jeffers
[b88999f] [PAY-2215] Play full purchased tracks from mobile library (#6821) Reed
[46c953e] Bump version to 0.5.29 audius-infra
[1d688a2] Track number of blobs served from mediorum and expose metric endpoint (#6816) Michelle Brier
[958ff2b] Fix merchant id (#6822) Raymond Jacobson
[d168c29] [C-3403] Fix sign-up e2e (#6815) Dylan Jeffers
[534a405] [PAY-2228] Add coinflow merchant id (#6820) Raymond Jacobson
[fe57135] Fix analytics after vite migration (#6819) Raymond Jacobson
[16fc48c] [C-3399] Fix trending links and twitter embed (#6818) Dylan Jeffers
[6517400] [C-3414] Update password reset modals to use harmony components (#6817) Kyle Shanks
[7f37ee2] Fix embed player playlist styles (#6813) Raymond Jacobson
[1e66d4f] Bump version to 0.5.28 audius-infra
[f519ae2] [PAY-2213] Fix closing Add Funds modal (#6814) Raymond Jacobson
[28d7f35] Set up test jail doc & skip current flaky tests (#6774) JD Francis
[0ca4675] Make DN a thin container when running locally, add createSender on DN start (#6335) Marcus Pasell
[8df08b3] trending: use aggregate tables (#6806) Steve Perkins
[16e2d08] [C-3389] Native harmony layout components (#6808) JD Francis
[e863af4] [C-3419] Add common sign-up components (#6809) Dylan Jeffers
[53772e6] [C-3415] Add confirm handle page (#6797) Dylan Jeffers
[3e82b69] Negative error codes for allowing (#6807) Isaac Solo
[1b7c76f] add arm builds for uptime (#6805) alecsavvy
[eadeadb] Update payment router readme (#6804) Saliou Diallo
[fc28641] [PAY-2208] Make payment router test e2e (#6803) Raymond Jacobson
[19eed3f] Bump version to 0.5.27 audius-infra
[d786864] [C-3391] Update native harmony Text component to use harmony theme values (#6798) Kyle Shanks
[40c3e18] [PAY-2211] Remove borders around withdraw/purchase tab empty states (#6796) Reed
[30a6674] Improve harmony font consistency (#6802) Raymond Jacobson
[8c377a4] Protocol dashboard entrypoint (#6776) Michelle Brier
[81ae2a1] [PAY-2219] Address Embed QA (#6801) Raymond Jacobson
[a5e5d56] Fix FixedDecimal.toShorthand() for numbers less than a tenth (#6742) Marcus Pasell
[00b0d25] Add Solana Relay to Discovery (#6782) Marcus Pasell
[f2a8150] Fix redirect protocol (#6800) Raymond Jacobson
[9ddbf19] [PAY-2220] Use slot_diff in user_bank health (#6799) Reed
[12f39f1] fix: Update welcome modal cover photo to use saved cover photos (#6781) JD Francis
[0c70996] [PAY-2216] DN health_check 500's if index user bank unhealthy (#6795) Reed
[7f35a50] Bump version to 0.5.26 audius-infra
[d8cf15c] require push-uptime to deploy (#6794) Michelle Brier
[76de38b] [C-3354, C-3396, C-3397] Sign up QA (#6780) Dylan Jeffers
[051c6c2] [PAY-2212] Don't grow fullWidth buttons (#6791) Reed
[5fe1c83] Fix uptime ts errors (#6793) Michelle Brier
[1124ae5] Fix dashboard prod deploy (#6787) Michelle Brier
[2fefdad] Enable embed ci workflow on main (#6792) Raymond Jacobson
[9b00279] [PROTO-1449] Make mediorum query eth directly for service providers (#6788) Theo Ilie
[5bc0a23] [PAY-2176] Implement remote config hooks via context (#6778) Randy Schott
[d734058] Default retry for verified user bot (#6783) Isaac Solo
[d58dbb4] [PAY-2210] Change crpyto transfer modal button to secondary (#6790) Reed
[9624aaa] Fix user bank indexing (#6789) Raymond Jacobson
[8448c37] [PAY-2214] Change require to import in AnimatedIconbutton (#6786) Reed
[26f89a8] Fix web lint (#6784) Reed
[22a34b1] [PAY-2066] Buy Crypto via SOL recovery flow (#6598) Marcus Pasell
[b47402a] Connect social to finish profile page, fix account header z index C-3384 (#6779) nicoback2
[d4d6b71] Bump version to 0.5.25 audius-infra
[c846f7a] d contracts interface (#6767) Michelle Brier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants