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

feat(DIA-927): conditionally display email enrollment checkbox during sign-up #11020

Merged
merged 3 commits into from
Oct 28, 2024

Conversation

iskounen
Copy link
Contributor

@iskounen iskounen commented Oct 25, 2024

This PR resolves DIA-927

Description

This PR updates the auth2 sign-up flow to conditionally display a checkbox that users can check to enroll in email subscriptions depending on whether it is required or not.

Required Not Required
Screenshot_1729885615 Screenshot_1729885602

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • conditionally display email enrollment checkbox during sign-up (auth2) - iskounen

Need help with something? Have a look at our docs, or get in touch with us.

@iskounen iskounen self-assigned this Oct 25, 2024
@iskounen iskounen marked this pull request as draft October 25, 2024 19:50
isAutomaticallySubscribed,
loading,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice and simple 👍


// TODO: Show a skeleton loader
if (loading) {
return null
Copy link
Member

Choose a reason for hiding this comment

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

This is the sort of thing where we should avoid using a skeleton because its so conditional. For most users, this just wont appear and we can probably just have it pop in. But def play with it! might be able to make it nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Thanks!

damassi
damassi previously approved these changes Oct 25, 2024
Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

👍 👍

@iskounen iskounen marked this pull request as ready for review October 28, 2024 16:58
@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Oct 28, 2024

This PR contains the following changes:

  • Dev changes (conditionally display email enrollment checkbox during sign-up (auth2) - iskounen - iskounen)

Generated by 🚫 dangerJS against be07d00


describe("user is automatically subscribed", () => {
beforeEach(() => {
;(useCountryCode as jest.Mock).mockReturnValue({
Copy link
Member

Choose a reason for hiding this comment

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

this is non-blocking, but in future code its helpful for future updates to do something like:

const mockUseCountryCode = useCountryCode as jest.Mock

up at the top

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'll make that change while I'm at it now 👍

damassi
damassi previously approved these changes Oct 28, 2024
@@ -21,5 +21,5 @@ appId: ${MAESTRO_APP_ID}
when:
platform: Android
commands:
- tapOn: "checkbox of consent, Check this element to consent with the Terms of Service"
- tapOn: "Accept terms and privacy policy, Check this element to accept Artsy's terms and privacy policy"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@araujobarret do you know what this file is for?

Copy link
Contributor

Choose a reason for hiding this comment

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

old stuff before maestro cloud, was supposed to be used in automation, I have to clean that up

Copy link
Member

Choose a reason for hiding this comment

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

I think this file was related to our test of a new E2E tool that we've abandoned. @brainbicycle / @gkartalis - right?

Copy link
Contributor

Choose a reason for hiding this comment

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

this one is @araujobarret work on their main offering which is more standard e2e tests, we might start looking into it again at some point but feel free to delete we can always bring it back

@iskounen iskounen merged commit 0361048 into main Oct 28, 2024
7 checks passed
@iskounen iskounen deleted the iskounen/feat/conditionally-display-email-checkbox branch October 28, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants