Skip to content

feat: add notice about registering with same email as sub #4491

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

Merged
merged 10 commits into from
May 16, 2025

Conversation

AmarTrebinjac
Copy link
Contributor

@AmarTrebinjac AmarTrebinjac commented May 15, 2025

Changes

Adds an alert to inform the user to use the same email for the registration as they did while subscribing.

I chose to use localStorage for this for two reasons.

  1. If they close the tab and reopen the session, it will persist.
  2. Putting it into queryparams seemed to need more tweaks than it was worth, when this works just as well in my opinion.
Screenshot 2025-05-15 at 15 47 55

(don't mind the bg image size, I just pasted in the desktop one cause its required in the editor 😂)

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Jira ticket

MI-899

Preview domain

https://mi-899.preview.app.daily.dev

Copy link

vercel bot commented May 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview May 16, 2025 4:24am
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) May 16, 2025 4:24am

@AmarTrebinjac AmarTrebinjac marked this pull request as ready for review May 15, 2025 04:35
@AmarTrebinjac AmarTrebinjac requested a review from a team as a code owner May 15, 2025 04:35
@@ -109,6 +109,13 @@ export const usePaddlePayment = ({
}),
});

if (router.pathname.includes('/helloworld')) {
localStorage.setItem(
'funnelSubscribed',
Copy link
Member

Choose a reason for hiding this comment

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

Let's store this string in a const to make it easier to be consistent across files 🙏

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Just one thing about the magic string, other than that, the Alert looks amazing 🤩

@@ -160,6 +161,8 @@ function InnerFunnelRegistration({
keepSession: true,
});

const didSubscribe = localStorage?.getItem('funnelSubscribed');
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if local storage is supported

Copy link
Contributor

Choose a reason for hiding this comment

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

we can also maybe add a fallback with query param, I know some instagram/facebook browsers sometimes disable local storage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@capJavert This was a good point. I think it's a bit redundant to use both, though, if queryparams is available in all cases (which I think it is), so I reverted from localStorage to queryparams entirely.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah query params can be a good choice since URL can't normally be changed on in-app browsers.

Base automatically changed from MI-882 to MI-881-sub-before-registration May 16, 2025 02:56
@AmarTrebinjac AmarTrebinjac merged commit ae25820 into MI-881-sub-before-registration May 16, 2025
10 checks passed
@AmarTrebinjac AmarTrebinjac deleted the MI-899 branch May 16, 2025 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants