-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat(PreviewTip): add preview tip #2923
Conversation
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.
Couple small comments!
@@ -82,6 +86,6 @@ describe('IconButton', () => { | |||
|
|||
userEvent.hover(cta); | |||
|
|||
view.getByText(tip); | |||
await waitFor(() => view.getByText(tip)); |
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.
nit: i think this is the same as doing await findByText(tip)
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.
since view isn't async, waitFor makes the functions wait around until it's true (or it times out)
)
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.
isnt that what findBy does https://testing-library.com/docs/dom-testing-library/api-async/#findby-queries
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.
omg TIL
return ( | ||
<GridBox | ||
gridTemplateAreas={gridTemplateAreas} | ||
gridTemplateColumns={avatar ? avatarColumnTemplate : '1fr'} |
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.
should the 1fr
here be using defaultGridTemplate
?
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.
good call, thank you!
overline="overline" | ||
href="/test" | ||
> | ||
I am a test tooltip |
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.
super nit: should this say preview tip instead?
{(args) => ( | ||
<FlexBox py={156} width="100%" justifyContent="space-around"> | ||
<PreviewTip {...args} truncateLines={2} alignment="top-right" /> | ||
<PreviewTip {...args} truncateLines={5} alignment="bottom-left" /> |
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.
do we want to change the lines here? truncating to 5 lines doesnt actually truncate with the current text
|
||
## Loading | ||
|
||
The `loading` prop will render a <LinkTo component="Shimmer">`Shimmer`</LinkTo> loading state. See an exmaple below! |
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.
lol example spelled wrong
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.
ex maple
<Story name="Loading PreviewTip"> | ||
{() => ( | ||
<FlexBox center py={156}> | ||
<LoadingTip /> |
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.
hmmm i'm not actually seeing the LoadingTip
here...
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.
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.
LGTM!
📬Published Alpha Packages:@codecademy/[email protected] |
🚀 Styleguide deploy preview ready! |
Overview
Add new PreviewTip component, restructures some shared
tip
components, and adds a 3ms delay to to hover/focus tips (like PreviewTip + ToolTip)PR Checklist
Testing Instructions
PR Links and Envs