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

feat: use a drawer for the streaks tooltip on mobile #2920

Merged
merged 2 commits into from
Apr 4, 2024
Merged

feat: use a drawer for the streaks tooltip on mobile #2920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3936ae6
feat: use a drawer for the streaks tooltip on mobile
DragosIorgulescu Apr 4, 2024
915fc33
Merge branch 'main' into MI-267-streaks-drawer-mobile
DragosIorgulescu Apr 4, 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
74 changes: 49 additions & 25 deletions packages/shared/src/components/streak/ReadingStreakButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import { isTesting } from '../../lib/constants';
import { useAnalyticsContext } from '../../contexts/AnalyticsContext';
import { AnalyticsEvent } from '../../lib/analytics';
import { useMobileUxExperiment } from '../../hooks/useMobileUxExperiment';
import { RootPortal } from '../tooltips/Portal';
import { Drawer } from '../drawers';
import ConditionalWrapper from '../ConditionalWrapper';

interface ReadingStreakButtonProps {
streak: UserStreak;
Expand Down Expand Up @@ -86,31 +89,52 @@ export function ReadingStreakButton({
}

return (
<Tooltip
content="Current streak"
streak={streak}
shouldShowStreaks={shouldShowStreaks}
setShouldShowStreaks={setShouldShowStreaks}
>
<Button
type="button"
icon={<ReadingStreakIcon secondary={hasReadToday} />}
variant={
isNewMobileLayout && isMobile
? ButtonVariant.Tertiary
: ButtonVariant.Float
}
onClick={handleToggle}
className={classnames('gap-1', compact && 'text-theme-color-bacon')}
size={
(isLaptop || !compact) && !isMobile
? ButtonSize.Medium
: ButtonSize.Small
}
<>
<ConditionalWrapper
condition={!isMobile}
Copy link
Member

Choose a reason for hiding this comment

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

Since there is content when it is not compact, I think we should also include the condition.

Suggested change
condition={!isMobile}
condition={!isMobile && compact}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow your comment here, the conditional wrapper is only for the Tooltip, which dictates wether it's wrapped around the button on non-mobile resolutions. It should have nothing to do with the compact property 🤔

Copy link
Member

@sshanzel sshanzel Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, but the thing is, we only display tooltip for buttons without text. Which when compact is applied, there is no text, and only when should be applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but tooltips are not a thing on mobile, right? so including compact here would not really matter.

Also, since the Tooltip or CustomStreaksTooltip is not dictated by compact, I feel like we could mistakenly create a situation where someone uses the compact version and ends up displaying both the main streaks tooltip and the drawer. I would suggest we tinker with this only if the situation arises that requires it, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay, I misunderstood, I thought this is only for tooltip to display a text. In that case, feel free to ignore and move on.

wrapper={(children: ReactElement) => (
<Tooltip
content="Current streak"
streak={streak}
shouldShowStreaks={shouldShowStreaks}
setShouldShowStreaks={setShouldShowStreaks}
>
{children}
</Tooltip>
)}
>
{streak?.current}
{!compact && ' reading days'}
</Button>
</Tooltip>
<Button
type="button"
icon={<ReadingStreakIcon secondary={hasReadToday} />}
variant={
isNewMobileLayout && isMobile
? ButtonVariant.Tertiary
: ButtonVariant.Float
}
onClick={handleToggle}
className={classnames('gap-1', compact && 'text-theme-color-bacon')}
size={
(isLaptop || !compact) && !isMobile
? ButtonSize.Medium
: ButtonSize.Small
}
>
{streak?.current}
{!compact && ' reading days'}
</Button>
</ConditionalWrapper>

{isMobile && (
<RootPortal>
<Drawer
displayCloseButton
isOpen={shouldShowStreaks}
onClose={handleToggle}
>
<ReadingStreakPopup streak={streak} fullWidth />
</Drawer>
</RootPortal>
)}
</>
);
}
12 changes: 11 additions & 1 deletion packages/shared/src/components/streak/popup/ReadingStreakPopup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, { ReactElement, useMemo } from 'react';
import { addDays, isSameDay, subDays } from 'date-fns';
import { useQuery } from '@tanstack/react-query';
import classNames from 'classnames';
import { StreakSection } from './StreakSection';
import { DayStreak, Streak } from './DayStreak';
import { generateQueryKey, RequestKey, StaleTime } from '../../../lib/query';
Expand All @@ -24,10 +25,12 @@ const streakDays = [

interface ReadingStreakPopupProps {
streak: UserStreak;
fullWidth?: boolean;
}

export function ReadingStreakPopup({
streak,
fullWidth,
}: ReadingStreakPopupProps): ReactElement {
const { user } = useAuthContext();
const { data: history } = useQuery(
Expand Down Expand Up @@ -83,7 +86,14 @@ export function ReadingStreakPopup({
<StreakSection streak={streak.current} label="Current streak" />
<StreakSection streak={streak.max} label="Longest streak 🏆" />
</div>
<div className="mt-6 flex flex-row gap-2">{streaks}</div>
<div
className={classNames(
'mt-6 flex flex-row gap-2',
fullWidth && 'justify-between',
Copy link
Member

Choose a reason for hiding this comment

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

We could've maybe used a className object prop here, but I really don't have a strong opinion here. Just makes it more flexible. Not necessary, this is optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since it's a classname specific to the streak days list, we would have to create a className prop which allows passing classnames to each section of the component internals. My suggestion would be to only do that if we need to control this component's UI further in the future 🙏

)}
>
{streaks}
</div>
<div className="mt-4 text-center font-bold leading-8 text-text-tertiary">
Total reading days: {streak.total}
</div>
Expand Down