feat: use a drawer for the streaks tooltip on mobile #2920
Conversation
DragosIorgulescu
requested review from
rebelchris,
omBratteng,
capJavert,
kopancek,
sshanzel and
nensidosari
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| } | ||
| <> | ||
| <ConditionalWrapper | ||
| condition={!isMobile} |
There was a problem hiding this comment.
Since there is content when it is not compact, I think we should also include the condition.
| condition={!isMobile} | |
| condition={!isMobile && compact} |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| <div | ||
| className={classNames( | ||
| 'mt-6 flex flex-row gap-2', | ||
| fullWidth && 'justify-between', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙏
Changes
Events
N / A
Manual Testing
On those affected packages:
Did you test the modified components media queries?
Did you test on actual mobile devices?
MI-267 #done
Preview domain
https://mi-267-streaks-drawer-mobile.preview.app.daily.dev