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

Interactions: Add step function and support multiple levels of nesting #18555

Merged
merged 51 commits into from
Aug 19, 2022

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Jun 23, 2022

What I did

Added a step function on the play function's context object. The step function effectively works like a Jest describe block, where you provide a label and a callback function as arguments:

Demo.play = async ({ args, canvasElement, step }) => {
  await step('One', async () => {
    // do things
  })
  await step('Two', async () => {
    // do more things
  })
}

Screen Shot 2022-06-23 at 20 39 43

Note that for the time being it's not possible to debug interactions inside a step function (similar to waitFor). That's out of scope for this PR.

How to test

  • Is this testable with Jest or Chromatic screenshots? yes
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? yes

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jun 23, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 26ec0e6. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ghengeveld ghengeveld marked this pull request as draft June 23, 2022 21:10
@ghengeveld ghengeveld marked this pull request as ready for review June 24, 2022 09:31
@ghengeveld
Copy link
Member Author

ghengeveld commented Jun 24, 2022

@tmeasday @shilman I'd like to get your 👍 on the decision to pass step on the play function's context argument. This way we make it a core part of the play function and avoid having to import it (I wouldn't know where from, except perhaps @storybook/instrumenter). The downside is that @storybook/preview-web gains a dependency on @storybook/instrumenter because of this.

@tmeasday
Copy link
Member

Hmm, I'm not sure we want to add that dependency. I wonder if we should have a (basically hidden) mechanism where addons can add fields to the story/play context?

Although even if we do that there's a typing issue (how does Story.play({ step }) get the right type?). It's tricky. WDYT @shilman?

@ghengeveld ghengeveld changed the title Interactions: Add step function and support multiple levels of nesting Interactions: Add step function and support multiple levels of nesting Jun 27, 2022
@ghengeveld
Copy link
Member Author

Hmm, I'm not sure we want to add that dependency. I wonder if we should have a (basically hidden) mechanism where addons can add fields to the story/play context?

What would that mechanism look like?

@tmeasday
Copy link
Member

OK, let's go for something in between:

  • We'll add an undocumented preset export step that ends up on the projectAnnotations, defaulting to f => f()
  • We'll add step to the PlayFunctionContext type
  • The preview will pass the projectAnnotations.step function to play().

How does that sound @ghengeveld?

@ghengeveld
Copy link
Member Author

ghengeveld commented Jun 28, 2022

  • We'll add an undocumented preset export step that ends up on the projectAnnotations, defaulting to f => f()

What happens if someone sets up another addon which exports a step value like that? Do we compose them? I suppose if we default it to f => f() we can just compose that with instrument.

@tmeasday
Copy link
Member

I was thinking it's just last one wins with maybe a warning. (Thus the undocumented).

Unless you think composing makes more sense?

@ghengeveld
Copy link
Member Author

@wKich this might be of interest to you.

@ghengeveld
Copy link
Member Author

This should be rebased off future/base

@yannbf
Copy link
Member

yannbf commented Jul 4, 2022

One note about this change: it will break @storybook/testing-react and the like. The mechanism to get the step function ideally should be simple so it can be automated without the user doing anything

@tmeasday
Copy link
Member

tmeasday commented Jul 4, 2022

@yannbf if the default is f => f() I don't think it'll break anything if it isn't explicitly set anywhere?

@ghengeveld ghengeveld force-pushed the ghengeveld/ap-1859-generic-step-function branch from 5caf14a to 7970c89 Compare August 10, 2022 11:29
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM!

@shilman
Copy link
Member

shilman commented Aug 12, 2022

@ghengeveld can you please review the chromatic change?

@ghengeveld ghengeveld requested a review from yannbf August 13, 2022 07:28
@ghengeveld
Copy link
Member Author

The debugger seems broken right now. Need to investigate.

@ghengeveld
Copy link
Member Author

@shilman This is good to go. Not sure what's up with cra-bench, seems like a Playwright or config issue unrelated to this PR.

@ghengeveld ghengeveld merged commit 2dc0cbe into next Aug 19, 2022
@ghengeveld ghengeveld deleted the ghengeveld/ap-1859-generic-step-function branch August 19, 2022 14:51
@IanVS
Copy link
Member

IanVS commented Sep 20, 2022

I suppose this needs to be added to the vite builder as well now, right?

@tmeasday
Copy link
Member

Oh! Yeah definitely. Did we miss this too? 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants