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.

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

Add interaction tests for pagelayout sticky #2224

Closed
wants to merge 30 commits into from

Conversation

pksjce
Copy link
Collaborator

@pksjce pksjce commented Aug 11, 2022

Add a couple of interactive tests

To test this, check PageLayout/interactions.stories

@pksjce pksjce requested review from colebemis and a team August 11, 2022 03:19
@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2022

🦋 Changeset detected

Latest commit: 4d75e24

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 74.21 KB (0%)
dist/browser.umd.js 74.75 KB (0%)

@pksjce pksjce temporarily deployed to github-pages August 11, 2022 03:26 Inactive
@pksjce pksjce temporarily deployed to github-pages August 11, 2022 03:27 Inactive
@colebemis colebemis mentioned this pull request Aug 11, 2022
7 tasks
@pksjce pksjce temporarily deployed to github-pages August 16, 2022 04:42 Inactive
@pksjce pksjce temporarily deployed to github-pages August 16, 2022 04:42 Inactive
Base automatically changed from pagelayout-sticky-prop to main August 16, 2022 06:22
@pksjce pksjce temporarily deployed to github-pages August 16, 2022 09:14 Inactive
import {within} from '@storybook/testing-library'
import {expect} from '@storybook/jest'

const meta: Meta = {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we can move this meta data out of the interaction stories and can be shared? As PageLayout.stories.tsx using the same data.

expect(isInViewPort(paragraphRect)).toBe(true)
const paragraph2 = await canvas.getByTestId('paragraph2')
const paragraphRect2 = paragraph2.getBoundingClientRect()
expect(isInViewPort(paragraphRect2)).toBe(false)
Copy link
Member

Choose a reason for hiding this comment

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

Cool! 🙌🏼

@colebemis
Copy link
Contributor

This is great. Thank you for setting this up, @pksjce! A couple of questions:

  • Do these interaction tests run during CI? Specifically, will CI fail if we accidentally break an interaction?
  • When I run this locally, is this what we expect the interaction tab to look like?

CleanShot 2022-08-17 at 12 32 35@2x

@pksjce
Copy link
Collaborator Author

pksjce commented Aug 18, 2022

I kinda messed up this branch badly. So I'll close this PR and open a new one.

@colebemis - Thanks!

  1. Yes they run on CI. On CI, playwright runs these "storybook tests". Will mention it in the new PR.
  2. The interactions for this test don't seem to be very expressive on the addon. There's also the problem that scrollIntoView does not come under any of the "actions" that this addon recognises. As mentioned in this https://storybook.js.org/addons/@storybook/addon-interactions at the very end, its

calls to userEvent., fireEvent, findBy, waitFor* and expect
that get shown in the step by step debugger.

However, I have some hope that this is being improved. In storybook 7.0.0 versions, there's improvements like this storybookjs/storybook#18555 , which help us have more expressive data in that addon. I feel that the addon is still new and not so mature yet.

@pksjce pksjce closed this Aug 18, 2022
@joshblack joshblack deleted the pk/pagelayout-sticky-prop branch January 19, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants