Tooltip/TooltipPrimitive: add visual tests #4508
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook staging is available at https://kiwicom-orbit-sarka-tooltip-visual-tests.surge.sh |
|
Size Change: 0 B Total Size: 459 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
e670af9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://60068fcd.orbit.pages.dev |
| Branch Preview URL: | https://sarka-tooltip-visual-tests.orbit.pages.dev |
sarkaaa
force-pushed
the
sarka/tooltip-visual-tests
branch
16 times, most recently
from
f3093c5 to
130f516
Compare
|
Hi guys, I would like to discuss some minor things - Tooltip visual tests. I’ve done some tests (tooltip/tooltip primitive), but the number of screenshots is pretty huge. I haven’t covered all of the cases (eg. all of the possible tooltip placements,..). Currently, I’m thinking about ways how to decrease the final number of screenshots.
Do you have any ideas/suggestions on how to reduce the amount of images? |
sarkaaa
force-pushed
the
sarka/tooltip-visual-tests
branch
6 times, most recently
from
2b40edf to
be815cb
Compare
sarkaaa
changed the title
sarkaaa
force-pushed
the
sarka/tooltip-visual-tests
branch
from
716ec19 to
a8f301e
Compare
DSil
reviewed
Nov 8, 2024
There was a problem hiding this comment.
I can’t include multiple tooltips in one screenshot, as the behavior is dependent on hover/click events
You can 😉 no need to use actions. You can open via props.
Exclude different placements at all and have only one placement (eg. bottom)
I think it's important to test all placements. Ideally we can have multiple placements in one screenshots (no need to have them all in one, but grouped somehow).
Exclude some of the viewports (eg. have only one mobile device and only two desktop viewports?)
That seems reasonable to me.
Exclude some variants - eg. block tooltip, with/without image
We don't need these. I'd say for TooltipPrimitive it's important to test: placement, size, error, help and removeUnderlinedText props. Same for Tooltip.
I'd also advise not to use an icon as the trigger, so we can actually test removeUnderlinedText.
| test(`screenshot for default - ${placement}`, async ({ mount }) => { | ||
| const component = await mount(<DefaultTooltipPrimitive placement={placement as Placement} />); | ||
|
|
||
| await component.getByRole("button").hover(); |
There was a problem hiding this comment.
We can avoid having the hover action. This behavior should be tested in unit tests, not visual tests
| placement={placement} | ||
| error={error} | ||
| help={help} | ||
| block={block} |
There was a problem hiding this comment.
If we add tooltipShown here, we don't need the hover
| return ( | ||
| <div | ||
| className={cx([ | ||
| "flex min-h-[128px]", |
There was a problem hiding this comment.
We can add some padding here, so the tooltip can "breathe" better. We probably need a bigger min-height as well.
There was a problem hiding this comment.
there was set bigger height before, I wanted to avoid "big blank spaces", but side-padding will be good (especially for small mobile viewport).
| ); | ||
| } | ||
|
|
||
| export function TooltipPrimitiveWithImage({ size = "medium" }: { size?: Size }) { |
There was a problem hiding this comment.
I feel we don't need this test with image at all
sarkaaa
force-pushed
the
sarka/tooltip-visual-tests
branch
from
a8f301e to
0e7aad2
Compare
sarkaaa
force-pushed
the
sarka/tooltip-visual-tests
branch
2 times, most recently
from
352e39c to
c83d59b
Compare
sarkaaa
commented
Nov 9, 2024
| export const resizeViewportHeight = async page => { | ||
| const viewportSize = page.viewportSize(); | ||
|
|
||
| if (viewportSize) { |
There was a problem hiding this comment.
This function sets a larger height of the viewport, without that, I couldn't cover all of the tooltip placements in one screen, eg: in the picture below, the last row of the tooltips are indented, because they were out of the default viewport height:
|
Thank you for the feedback, currently I covered these variants:
(basic) Tooltip component test covers only default tooltip. This tooltip is a wrapper of primitiveTooltip (without some props - tooltipShown, error, help,..), and that's the reason why no all of the variants mentioned above are not covered in these test file. The main difference is how the tooltip is shown on mobile devices. |
|
LGTM, but I think we don't need to test all the placements for all the variations. So, for small size, error, help and noUnderline I'd say we only need one tooltip. This way we can have much smaller snapshots (bigger snapshots are more likely to give false results) |
Yeah, I agree with the statement that we don't need test all plasements for variations (noUderline, error, etc..). I'll exclude them, and the size of the snapshots will be smaller. |
sarkaaa
force-pushed
the
sarka/tooltip-visual-tests
branch
from
c83d59b to
521b0ed
Compare
sarkaaa
force-pushed
the
sarka/tooltip-visual-tests
branch
2 times, most recently
from
63d729d to
d21bd67
Compare
sarkaaa
force-pushed
the
sarka/tooltip-visual-tests
branch
from
8588df7 to
e670af9
Compare
DSil
approved these changes
Nov 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.ts