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

Clear storyStatus when no longer relevant #65

Merged
merged 12 commits into from
Sep 4, 2023
Merged

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Aug 31, 2023

What I did:

  • clear storyStatus when we get partial buildData
  • clear storyStatus when there is no build
📦 Published PR as canary version: 0.0.53--canary.65.575bce7.0

✨ Test out this PR locally via:

npm install @chromaui/addon-visual-tests@0.0.53--canary.65.575bce7.0
# or 
yarn add @chromaui/addon-visual-tests@0.0.53--canary.65.575bce7.0

@ndelangen ndelangen self-assigned this Aug 31, 2023
@ndelangen ndelangen added the bug Classification: Something isn't working label Aug 31, 2023
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. Is it possible to do some assertion a play function that we call the right version of updateStatus at the right time in the a VisualTests story?

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.

Great job on the tests ✅ I have a couple questions before we merge this though

package.json Outdated Show resolved Hide resolved
src/screens/VisualTests/VisualTests.stories.tsx Outdated Show resolved Hide resolved
src/screens/VisualTests/VisualTests.tsx Outdated Show resolved Hide resolved
src/screens/VisualTests/VisualTests.stories.tsx Outdated Show resolved Hide resolved
@ndelangen
Copy link
Member Author

ndelangen commented Sep 4, 2023

@gert the custom render method is easier way to override the arg.
You suggested adding a arg with a _ prefix, remember? I think this is a much much cleaner solution.

In fact the _ prefix method didn't work, this one does.

EDIT: never mind, the suggestion by @tmeasday worked.

@ndelangen
Copy link
Member Author

ndelangen commented Sep 4, 2023

Scrap that, that solution doesn't work... @tmeasday

Do you have suggestions that would fix the problem then, considering you do not like the custom render solution.

@tmeasday
Copy link
Member

tmeasday commented Sep 4, 2023

@ndelangen let me try

@tmeasday
Copy link
Member

tmeasday commented Sep 4, 2023

@ndelangen this approach works although has some typing issues that may be resolvable:

  args: {
    updateBuildStatus: action("updateBuildStatus"),
  },
  play: async ({ args }) => {
    await waitFor(() => {
      expect(args.updateBuildStatus).toHaveBeenCalled();
      const fn = args.updateBuildStatus.mock.calls[0][0];
      expect(fn({})).toEqual({});
    });
  },

WDYT?

@ndelangen
Copy link
Member Author

ndelangen commented Sep 4, 2023

I think the custom render approach is a simpler approach @tmeasday and has no type issues..

That proposal has type issues on both the args and play function side.
action doesn't the type of the arg.
If I force it than the args that the play function receives won't match the jest mock..

I could force both.. but why?
A customer render function solves it, without any type casting. It's AFAICS far more type-safe / type-correct.

@ndelangen ndelangen merged commit cf907fc into main Sep 4, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Classification: Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants