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.

Sign up for GitHub

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

Jump to bottom

feat: streak warning prompt for timezone #4071

Merged
merged 8 commits into from
Jan 16, 2025
Merged

feat: streak warning prompt for timezone #4071

merged 8 commits into from
Jan 16, 2025

Conversation

capJavert
Copy link
Contributor

@capJavert capJavert commented Jan 16, 2025 •
edited
Loading

Changes

  • warning is now shown when device timezone does not match user setting
  • see thread for designs
  • check is done over offset which should be better then timezone string (since multiple timezones can have the same offset)
    • this should also work good when DST is in effect 🤞
  • user can choose to ignore the warning which is then saved to device storage and user is no longer prompted
    • evaluated to save it on API but for simplicity sake I think its fine to do it on device

Events

Did you introduce any new tracking events?

Log the new/changed events below:

Type event_name value
Change click target_type: streak timezone label, extra: { device_timezone, user_timezone, timezone_ok, timezone_ignore }
Change click target_type: streak timezone mismatch prompt, extra: { device_timezone, user_timezone, timezone_ok, timezone_ignore }
New streak timezone mismatch extra: { device_timezone, user_timezone, timezone_ok, timezone_ignore }
  • device_timezone - device timezone detected
  • user_timezone - current user timezone
  • timezone_ok - do timezones match
  • timezone_ignore - if user had ignore preference set

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

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

AS-914 #done

Preview domain

https://as-914-reading-streak-warning-pr.preview.app.daily.dev

@capJavert capJavert self-assigned this Jan 16, 2025
@vercel Vercel
Copy link

vercel bot commented Jan 16, 2025 •
edited
Loading

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

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Jan 16, 2025 1:55pm
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Jan 16, 2025 1:55pm

@capJavert capJavert changed the base branch from main to AS-914-reading-streak-fix-1 January 16, 2025 11:12
@capJavert capJavert changed the title As 914 reading streak warning prompt feat: streak warning prompt for timezone Jan 16, 2025
@AmarTrebinjac
Copy link
Contributor

AmarTrebinjac commented Jan 16, 2025 •
edited
Loading

Tested it, and the logic seems to work correctly, but a couple of things:

  1. I don't like that when the modal is open, clicking anywhere on the background closes it and clears the warning triangle, never to be seen again before you change the timezone again. One can click this away easily as a whoopsie (I did, just now) and it could confuse the user.

  2. I'm not entirely sure what I think about this:
    image

I think it looks... weird. 😅 What do you think about maybe making the warning triangle the icon instead of the streak? Would make it instantly more visible too, I think..

EDIT: Also, once again I have been baited by the DRAFT 😡

@capJavert
Copy link
Contributor Author

capJavert commented Jan 16, 2025 •
edited
Loading

@AmarTrebinjac 😆

good feedback on 1. point, I fixed it so it only closes if you select something

capJavert
capJavert commented Jan 16, 2025
View reviewed changes
packages/shared/src/components/icons/Warning/filled.svg Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason the old icon was broken, we did not use it anywhere else in the project

capJavert
capJavert commented Jan 16, 2025
View reviewed changes
packages/shared/src/components/modals/Prompt.tsx
@@ -46,6 +47,7 @@ export function PromptElement(props: Partial<ModalProps>): ReactElement {
overlayClassName="!z-max"
isDrawerOnMobile
drawerProps={{ displayCloseButton: false, appendOnRoot: true }}
shouldCloseOnOverlayClick={shouldCloseOnOverlayClick}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding support to disable outside click on prompt

capJavert
capJavert commented Jan 16, 2025
View reviewed changes
packages/shared/src/components/streak/ReadingStreakButton.tsx
<IconWrapper wrapperClassName="relative flex items-center gap-2">
<ReadingStreakIcon secondary={hasReadToday} />
{!isTimezoneOk && (
<WarningIcon className="text-raw-cheese-40" secondary />
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 want this specific color

capJavert
capJavert commented Jan 16, 2025
View reviewed changes
packages/shared/src/components/streak/popup/ReadingStreakPopup.tsx
@@ -82,10 +99,13 @@ interface ReadingStreakPopupProps {
fullWidth?: boolean;
}

const timezoneSettingsHref = '/account/notifications?s=timezone';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just in case we want to do something special when user comes from timezone prompt in the future

capJavert
capJavert commented Jan 16, 2025
View reviewed changes
packages/shared/src/hooks/streaks/useStreakTimezoneOk.ts
Comment on lines +36 to +37
// once off check to see how many users with timezone mismatches we have in the wild
useEffect(() => {
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 can also log this every time but for now it felt it was too much because for users that had the wrong timezone it would log every time they open the app

@capJavert capJavert marked this pull request as ready for review January 16, 2025 13:52
@capJavert capJavert requested a review from a team as a code owner January 16, 2025 13:52
@capJavert
Copy link
Contributor Author

@idoshamun to check events 🙏

idoshamun
idoshamun reviewed Jan 16, 2025
View reviewed changes
packages/shared/src/lib/log.ts
@@ -170,6 +170,7 @@ export enum LogEvent {
ScheduleStreakReminder = 'schedule streak reminder',
StreakRecover = 'restore streak',
DismissStreakRecover = 'dimiss streaks milestone',
StreakTimezoneMismatch = 'streak timezone mismatch',
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between this and the click with "streak timezone mismatch prompt"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@idoshamun this is (currently) a once off to see how many users will even see the warning, it is once of for now to not spam #4071 (comment)

rebelchris
rebelchris approved these changes Jan 16, 2025
View reviewed changes
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.

Looks good to me :)

@capJavert capJavert merged commit 19fcf7e into AS-914-reading-streak-fix-1 Jan 16, 2025
11 checks passed
@capJavert capJavert deleted the AS-914-reading-streak-warning-prompt branch January 16, 2025 14:21
@capJavert capJavert mentioned this pull request Jan 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rebelchris rebelchris rebelchris approved these changes

@idoshamun idoshamun idoshamun approved these changes

@AmarTrebinjac AmarTrebinjac AmarTrebinjac approved these changes

@omBratteng omBratteng Awaiting requested review from omBratteng omBratteng was automatically assigned from dailydotdev/web-team

@ilasw ilasw Awaiting requested review from ilasw ilasw was automatically assigned from dailydotdev/web-team

@sshanzel sshanzel Awaiting requested review from sshanzel sshanzel was automatically assigned from dailydotdev/web-team

@nensidosari nensidosari Awaiting requested review from nensidosari nensidosari was automatically assigned from dailydotdev/web-team

Assignees

@capJavert capJavert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@capJavert @AmarTrebinjac @rebelchris @idoshamun