chromaui / addon-visual-tests

Visual Tests addon for Storybook
MIT License
33 stars 2 forks source link

Allow setting builds as args rather than parameters #90

Closed tmeasday closed 11 months ago

tmeasday commented 1 year ago

Trying this out - is this what you had in mind @yannbf? It seems like the types don't like it though @kasperpeulen (see the @ts-expect-error, not quite sure why. I can set args.AddonVisualTestsBuild in the meta but not in the story.

It does work though.

image

📦 Published PR as canary version: 0.0.64--canary.90.6a458fa.0
:sparkles: Test out this PR locally via: ```bash npm install @chromaui/addon-visual-tests@0.0.64--canary.90.6a458fa.0 # or yarn add @chromaui/addon-visual-tests@0.0.64--canary.90.6a458fa.0 ```
kasperpeulen commented 1 year ago

Really nice to see that args can be used for together with msw, would make it much more powerful!

tmeasday commented 11 months ago

Closing this in favour of a similar change in https://github.com/chromaui/addon-visual-tests/pull/124

Note in that PR I nested the args under $graphql:

https://github.com/chromaui/addon-visual-tests/blob/fcaf963a785e8a10c969d7b520054993882a825a/src/screens/VisualTests/VisualTests.stories.tsx#L281-L298

I also add a "mapping" concept:

https://github.com/chromaui/addon-visual-tests/blob/fcaf963a785e8a10c969d7b520054993882a825a/src/screens/VisualTests/VisualTests.stories.tsx#L136-L141

https://github.com/chromaui/addon-visual-tests/blob/fcaf963a785e8a10c969d7b520054993882a825a/src/screens/VisualTests/VisualTests.stories.tsx#L101-L125

It works pretty well. @shilman I think nesting under $graphql makes it a little more intuitive to read, but a bit more awkward to write--ie:

export const StoryAddedNotInBuild = {
  args: {
    $graphql: {
      AddonVisualTestsBuild: {
        selectedBuild: withTests({ ...pendingBuild }, []),
      },
    },
  },
}

// vs 

export const StoryAddedNotInBuild = {
  args: {
    AddonVisualTestsBuild: {
      selectedBuild: withTests({ ...pendingBuild }, []),
    },
  },
}

On the one hand the extra level of nesting is just more work all the time, on the other hand it makes it much more clear what this arg that has nothing to do with the component's props is doing.

shilman commented 11 months ago

Haven't thought this through, but I wonder if we could just hoist $graphql and make $x the convention for targeted args, reducing one level of hierarchy?

tmeasday commented 11 months ago

@shilman we could but I guess I was saying the $graphql is actually useful because at a glance it distinguishes the target of the args when you read the story and don't have all the context.

If it was $AddonVisualTestsBuild it would still be clear it's not a prop, but not where it's going.

Maybe that's a good tradeoff though? I guess in any given project you are probably only going to have one or two arg "targets".

If we did it that way, we could even just make the $- thing a convention, given you'll have to define somewhere that $AddonVisualTestsBuild targets graphql.

shilman commented 11 months ago

Sorry I meant the other way. Pull $graphql to be a peer of args.

tmeasday commented 11 months ago

So a new annotation?

yannbf commented 11 months ago

I think it makes sense to have them under args:

  1. People already connect args to the controls table, or addons that provide a controls-like experience
  2. You still maintain a scope, rather than having a freeform property in the story top annotation level
  3. The $ prefix (maybe it should be $$) helps distinguish what is not a prop, the only framework that prepends certain properties with $ is angular, for observables
shilman commented 11 months ago

Yeah a new class of annotation. Where we could have $graphql, $mock, $<yourTargetHere>.

I don't love it, but I'm bringing it up because we always try to be careful about scoping things correctly and then we generate these deeply nested structures that are not that ergonomic, and then we end up hoisting them later. But maybe we should consider just hoisting from the beginning under the assumption that targeted args will be a big deal and people will be using them a lot.