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

fix: date comparison without timezone in consideration #3832

Merged
merged 8 commits into from
Nov 21, 2024
Merged

fix: date comparison without timezone in consideration #3832

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5919744
fix: date comparison without timezone in consideration
sshanzel Nov 18, 2024
d0dcb15
Merge branch 'main' into MI-620
omBratteng Nov 19, 2024
23626bb
refactor: usage of lastviewat tz
sshanzel Nov 20, 2024
2656bd9
fix: app crash
sshanzel Nov 20, 2024
65c8b06
Merge branch 'main' into MI-620
sshanzel Nov 20, 2024
08c8497
Merge branch 'main' into MI-620
sshanzel Nov 20, 2024
b96b882
Merge branch 'main' into MI-620
sshanzel Nov 20, 2024
480f1e6
Merge branch 'main' into MI-620
sshanzel Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions packages/shared/src/components/streak/ReadingStreakButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { ReactElement, useCallback, useState } from 'react';
import classnames from 'classnames';
import { utcToZonedTime } from 'date-fns-tz';
import { ReadingStreakPopup } from './popup';
import {
Button,
Expand All @@ -18,6 +19,7 @@ import { RootPortal } from '../tooltips/Portal';
import { Drawer } from '../drawers';
import ConditionalWrapper from '../ConditionalWrapper';
import { TooltipPosition } from '../tooltips/BaseTooltipContainer';
import { useAuthContext } from '../../contexts/AuthContext';

interface ReadingStreakButtonProps {
streak: UserStreak;
Expand Down Expand Up @@ -63,6 +65,20 @@ function CustomStreaksTooltip({
);
}

const DEFAULT_TIMEZONE = 'UTC';
const getHasReadToday = (
lastView: Date | null,
timezone = DEFAULT_TIMEZONE,
) => {
if (!lastView) {
return false;
}

const lastViewTz = utcToZonedTime(new Date(lastView), timezone);

return lastViewTz.getDate() === new Date().getDate();
};

export function ReadingStreakButton({
streak,
isLoading,
Expand All @@ -71,11 +87,11 @@ export function ReadingStreakButton({
className,
}: ReadingStreakButtonProps): ReactElement {
const { logEvent } = useLogContext();
const { user } = useAuthContext();
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);
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

@sshanzel sshanzel Nov 21, 2024 •
edited
Loading

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 🙏


const handleToggle = useCallback(() => {
setShouldShowStreaks((state) => !state);
Expand Down