Controls: Add conditional controls

[shilman]

Issue: #11984

What I did

Revive & update #13890

This PR adds/removes ArgsTable rows and their corresponding Args data conditionally based on the value of other args:

argTypes: {
  baz: { addIf: 'foo' } // add when foo is truthy
  moo: { removeIf: 'foo' } // remove when foo is falsey
}

Some examples:

How to test

See updated stories & unit tests

[nx-cloud]

☁️ Nx Cloud Report

CI ran the following commands for commit 1a37df7. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 1 target

• nx run-many --target=prepare --all --parallel --max-parallel=15 -- --optimized

---

Sent with 💌 from NxCloud.

[shilman]

Other cases:

• hidden
• readonly

[yannbf]

Nice stuff! I wonder about the relation from this change to
https://storybook.js.org/docs/react/essentials/controls#filtering-controls and if we need to differentiate them to avoid confusion. The controls.include|exclude was introduced to make table.disable simpler and allow for regex based exclusion (though I believe it wouldn't affect the argstable in docs so that's one key difference)

Maybe we should rename the new ones to like allow/reject, add/remove, keep/?

[yannbf]

Hey @tmeasday just to verify, the snapshots are failing because the play fn was removed in this PR in composeConfigs. This was a followup to a change made in CSF types which did not include the play function in ProjectAnnotations anymore. It's intended as we don't support project-level play fn, right? So we can safely update the snapshots

[tmeasday]

It's intended as we don't support project-level play fn, right?

I think that sounds right, what do you think? Can't think of any reason you'd want one.

[yannbf]

It's intended as we don't support project-level play fn, right?

I think that sounds right, what do you think? Can't think of any reason you'd want one.

Nah it was just to double check before removing the code!

[kylegach]

Other cases:

• hidden
• readonly

What about including/excluding when another arg is a certain value, e.g. only render color when appearance = 'primary' (where appearance has options defined)?

API idea:

argTypes: {
  appearance: {
    options: ['primary', 'outline'],
  },
  color: { includeIf: ['appearance', 'primary'] } // enable when appearance = 'primary'
}

[shilman]

@yannbf @tmeasday responded to first round of review comments including rename from includeIf/excludeIf to addIf/removeIf. Not overjoyed with where things are, but also not sure the best way forward.

[yannbf]

@yannbf @tmeasday responded to first round of review comments including rename from includeIf/excludeIf to addIf/removeIf. Not overjoyed with where things are, but also not sure the best way forward.

Care to elaborate? How can we help?

[shilman]

@yannbf I'm not happy about the API for hiding controls. Currently there are two ways to do this:

1. the controls.include/exclude parameter
2. the table.disable argType annotation

@tmeasday and I had discussed a hide: true argType annotation that would replace/simplify table.disable and I feel this is a more descriptive name than exclude, since it is just hiding it in the UI and not in the args data. But the name inconsistency with controls.disable bothers me.

Some options:

1. Do nothing (this PR)
2. Rename controls.include/exclude to controls.show/hide and add argsType.hide
3. Introduce argType.exclude instead of argType.hide

Another option would be to move all this under argType.control which would namespace all this and make it a little more symmetrical with the controls parameter for global inclusion/exclusion. But would require more typing for the user.

TLDR: I feel the current naming is inconsistent and it would be nice to agree on some conventions here since we're going to have to live with this for years.

[yannbf]

@yannbf I'm not happy about the API for hiding controls. Currently there are two ways to do this:

1. the controls.include/exclude parameter
2. the table.disable argType annotation

@tmeasday and I had discussed a hide: true argType annotation that would replace/simplify table.disable and I feel this is a more descriptive name than exclude, since it is just hiding it in the UI and not in the args data. But the name inconsistency with controls.disable bothers me.

Some options:

1. Do nothing (this PR)
2. Rename controls.include/exclude to controls.show/hide and add argsType.hide
3. Introduce argType.exclude instead of argType.hide

Another option would be to move all this under argType.control which would namespace all this and make it a little more symmetrical with the controls parameter for global inclusion/exclusion. But would require more typing for the user.

TLDR: I feel the current naming is inconsistent and it would be nice to agree on some conventions here since we're going to have to live with this for years.

I'm fine with option 2 and would be happy to write a codemod for it.

There's also another option: use both the concept of exclude/include and hide/show, both doing what they semantically mean.

I'm not sure if there is a use case for this, but just want to throw it out there for us to consider: With targetedArgs one might also want to tweak it in the UI. For example an addon that requires 5 args but the user only wants to allow changing one. In that case hide/show or enabled:true|false would be needed in argTypes.

I guess things might get more convoluted and we might need to have the "if" variants for show, hide, include, exclude, enable, disable. Maybe another spin to this is to change the api to become more flexible like:

{
  conditional: {
    condition: "foo",
    not: true,
    type: "hide" // or show, exclude, include, enable, disable
  }
}

[tol-is]

Hi, it would be great to have this feature. Great work so far, and to be honest, I would manage to work with any of the patterns proposed.

Perhaps I'm late to suggest another option, but worth sharing, just in case. Disclaimer, I have no understanding whatsoever of how storybook's wiring.

The pattern is basically out of https://github.com/pmndrs/leva. Essentially, each key could have a hidden key, that's either null, a boolean, or a function, which receives all the values, and returns a boolean.

It can be used for hidden and readonly, or disabled flags.

argTypes: {
  // always hidden
  type: {
    options: ['primary', 'outline'],  
    hidden: true
  },
  shape: {
      options: ["tomato", "potato"],
      control: { type: "radio" },
  },
  // hide if shape is 'potato'
  color: {
     options: ["red", "green"],
     hidden: (params) => params.shape === 'potato'
  }
}

Cheers

[kylegach]

The pattern is basically out of https://github.com/pmndrs/leva. Essentially, each key could have a hidden key, that's either null, a boolean, or a function, which receives all the values, and returns a boolean.

That's a much better API for non-boolean cases than the one I proposed. 👍

[shilman]

@a7sc11u @kylegach unfortunately functions are currently problematic in storybook since (1) they need to be serialized over the channel, (2) they are impossible for ecosystem tools to statically analyze, so that's why we're using an awkward declarative format.

[ethriel3695]

So, users have to opt into the advanced controls in this case right?

[ethriel3695]

Does this mean we no longer have to pass diable to the table object?
We will use removeIf and addIf everywhere?

[shilman]

self-merging @tmeasday @yannbf to get this unstuck. still did not figure out the various permutations of visibility & read-only etc but i've timeboxed this and will come back to it

@ethriel3695 i've reverted the removeIf: true piece. we'll continue to use table.disable for now until we have something better

[GeorgiMoskov]

Unfortunately this still doesn't enable us to use dynamic controls in their full potential.
As example in one of our projects we have a global theme provider.
Different themes stores different variant keys for each component.
So previously we would extract those variants from the current active theme and pass them to the select knob.
But now we are not able to do that.

[shilman]

@GeorgiMoskov please check out the reworked version #17883 -- you might be able to hack something because it does allow to show & hide based on the state of globals. it won't do exactly what you did in dynamic knobs, but it's not intended to replace fully dynamic knobs. rather, it is just enabling some of the more common use cases.

[GeorgiMoskov]

@shilman If it's not intended to fully replace the dynamic knobs but knobs are being deprecated. What is the alternative of the dynamic knobs then? As you mentioned this solutions does not actually close the issue it is just adding a conditional logic over static values. Can you give us an example how can we "hack"/implement such functionality?