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: onboarding plus step #3869

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

feat: onboarding plus step #3869

wants to merge 38 commits into from

Conversation

ilasw
Copy link
Contributor

@ilasw ilasw commented Nov 22, 2024

Changes

  • Add new step for Plus subscription during onboarding

Events

Did you introduce any new tracking events?

image

Experiment

Not anymore, according to #experiments channel thread

Manual Testing

Caution

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

Preview domain

https://feat-onboarding-plus-step.preview.app.daily.dev

Copy link

vercel bot commented Nov 22, 2024

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

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Nov 27, 2024 3:36pm
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Nov 27, 2024 3:36pm

@ilasw ilasw changed the title feat: WIP onboarding plus step feat: onboarding plus step Nov 22, 2024
@@ -224,6 +230,15 @@ export function OnboardPage(): ReactElement {
return setActiveScreen(OnboardingStep.Sources);
}

if (
(showOnboardingSources && activeScreen === OnboardingStep.Sources) ||
Copy link
Member

Choose a reason for hiding this comment

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

Is this not going to be behind any existing experiment we have for plus subscription?

Copy link
Contributor

Choose a reason for hiding this comment

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

sources is a running experiment, you can't rely on it yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should actually be it's own evaluation experiment

];

export const plusFeatureList = [
'Ads-free browsing',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Ads-free browsing',
'Everything on Free plan',
'Ads-free browsing',

return (
<li className="flex gap-2">
<ChecklistAIcon className="text-text-quaternary" size={IconSize.Small} />
<ChecklistAIcon
className="text-text-quaternary"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about other places, but here, I think the color should be tertiary.

Screenshot 2024-11-23 at 2 05 43 PM

logSubscriptionEvent: (event: LogSubscriptionEvent) => void;
} => {
const { user } = useAuthContext();
const isPlus = user?.isPlus || false;
const plusSubscriptionFeature = useFeature(feature.plusSubscription);
const isOnboardingPlusActive = useFeature(feature.onboardingPlus);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh the only issue here is that it will enroll everyone as soon as they hit onboarding page, and actually anytime this hook gets called even.

Did you check enrollment with ido in #experiment channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated ✔️

Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Nice, just one minor change on enums for logging

if (isOnboardingPlusActive) {
logSubscriptionEvent({
event_name: 'skip upgrade subscription',
target_id: 'onboarding',
Copy link
Contributor

Choose a reason for hiding this comment

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

Both these should be enums in log file

Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Approving already, but we should have onboarding already its not a new thing

@@ -281,6 +284,7 @@ export enum TargetId {
Ads = 'ads',
MyProfile = 'my profile',
PlusBadge = 'plus badge',
Onboarding = 'onboarding',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have this already, but maybe not id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have it on Origin, should I use this instead?

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.

3 participants