test(skeleton): Add stories and enabled snapshots #720
Conversation
mannycarrera4
changed the title
|
Thoughts on the way it's being broken up? |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
modules/skeleton/react/stories/visual-testing/stories_Skeleton.tsx
Outdated
Show resolved
Hide resolved
mannycarrera4
added
on hold
| export const SkeletonStates = () => ( | ||
| <div> | ||
| <h3>With Custom Line Count</h3> | ||
| <StaticStates> |
There was a problem hiding this comment.
StaticState isn't used anywhere. This only does something if className is used as a prop and only for pseudo states like .focus or .hover.
I agree with @lychyi where the table isn't necessary to show all component states - especially in this example where there isn't any matrix. But it does show nice text descriptions of what we're looking at. I think visual tests, like our functional tests, should have context easily available. The way the text is laid out helps describe what's in the visual snapshot.
The confusing part here is which skeleton types are being used.
First table is SkeletonText, but the label says "With Custom Line Count" even though the "Default" has no custom line count. Your variant would be your line count of 4.
Second table is SkeletonShape. with a few different widths, heights, and border radii.
Third table has SkeletonShape, SkeletonHeader, and SkeletonText. I can see by the story that SkeletonHeader is not obvious and makes me wonder if there are different header types (heights). I'd like to see side-by-side comparisons of skeletons to actual content. A common thing I see people get wrong with skeleton loading is that the "skeleton" isn't actually a skeleton of the layout - so layout shifts. Sometimes dramatically which defeats the entire purpose of skeleton loaders. We should have stories that show what they look like side-by-side so we can see that they line up correctly. We should have some stories that show loading at different loading times (best practices of when to show them) with the ability to reset.
Skeleton loaders are abused so many times and we don't help the situation since we don't provide any good examples.
mannycarrera4
added
ready for review
…canvas-kit into mc-skeleton-snaps
| @@ -13,7 +13,7 @@ export interface SkeletonProps { | |||
|
|
|||
| const TRANSPARENCY_POSITION = 45; | |||
| const WHITE_SHEEN_WIDTH = 10; | |||
| const DURATION = 5; | |||
| const DURATION = 2; | |||
There was a problem hiding this comment.
Did you meant to change this value?
There was a problem hiding this comment.
Yes, i ran it by the design team in PT, but Alex brought up a point that it might behave weird on larger screens?
There was a problem hiding this comment.
I think changing that value should be from a GH issue explaining why it needs to change. Otherwise that context gets lost. There are so many things that exist without any context.
There was a problem hiding this comment.
that's understandable, I'll revert this change until we have some more context
There was a problem hiding this comment.
I found this: #134
I still think changing this value should be a different PR addressing this issue as well as a rationale for what the final number becomes.
Summary
Fixes #717
Partially fixes: #683
Add snapshots for skeleton component