-
Notifications
You must be signed in to change notification settings - Fork 229
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
fix: date comparison without timezone in consideration #3832
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of a question why can't we handle this in API returning the value we expect?
const isLaptop = useViewSize(ViewSize.Laptop); | ||
const isMobile = useViewSize(ViewSize.MobileL); | ||
const [shouldShowStreaks, setShouldShowStreaks] = useState(false); | ||
const hasReadToday = | ||
new Date(streak?.lastViewAt).getDate() === new Date().getDate(); | ||
const hasReadToday = getHasReadToday(streak?.lastViewAt, user?.timezone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But shouldn't API just return it in the correct format already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, what is returned is the stored value, which is UTC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can maybe add a prop where the returned value involves the right tz.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced an API change for the timezome-adjusted value: dailydotdev/daily-api#2471
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So then we don't need this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For backwards compatibility, I have kept the original column, and introduced a new one with timezone considered. So we'd still need this PR 🙏
Changes
lastViewAt
is in UTC when fetched from the API, which does not match the local time. With the update, we will utilize the user's set timezone. We can also use the current local timezone but it won't match other stats since behind the scenes all we use is the set timezone from the profile.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
Preview domain
https://mi-620.preview.app.daily.dev