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

Change knobs via actions [idea] #3855

Closed
IanVS opened this issue Jul 9, 2018 · 55 comments
Closed

Change knobs via actions [idea] #3855

IanVS opened this issue Jul 9, 2018 · 55 comments

Comments

@IanVS
Copy link
Member

IanVS commented Jul 9, 2018

I've just started using storybook and so far I love it. One thing I've struggled with is how to deal with pure "stateless" components which require state to be contained in a parent. For example, I have a checkbox which takes a checked prop. Clicking the checkbox does not toggle its state, but rather fires an onChange, and waits to get an updated checked prop back. There doesn't seem to be any documentation on best practices to handle these kinds of components, and the suggestions in issues such as #197 have been to create a wrapper component or to add another add-on. I would rather not make a component wrapper if I can help it, because I'd like to keep my stories as simple as possible.

One idea I had to handle this is to wire up the actions add-on with knobs, allowing knobs to be toggled programmatically via actions. As I said I'm brand-new at storybook, so I have no idea whether this is even feasible, but I wanted to at least raise the suggestion.

This is a stumbling point for me in implementing stories, and I imagine it might be for others as well. If creating a wrapper component really is the best thing to do, perhaps some documentation could be added to clarify this, and how to accomplish it?

@igor-dv
Copy link
Member

igor-dv commented Jul 9, 2018

Some kind of similar idea was introduced in this #3701 PR, that was controversial (and was not merged).

We can open a discussion about it again, and listen to the API suggestions =).

@IanVS
Copy link
Member Author

IanVS commented Jul 9, 2018

Ah thank you, I hadn't seen that PR. Thanks @aherriot for putting that together, it seems we have the same thoughts.

Before diving into an API, it seems like the fundamental concept needs to be discussed and agreed on. One of the comments in the PR was this from @Hypnosphi:

I don't like the fact that it introduces multiple sources of truth (component callbacks and UI knobs)

It seems to me that the idea is not to introduce different sources of truth, but rather to allow maintaining a single source of truth for component state—the knobs. All of the other approaches I've seen suggested (wrapper components, state add-ons, recompose) would in fact introduce another source of truth. In my checkbox example, I would not be able to have a knob for checked, and also allow a wrapper component to provide the checked prop. I see the knobs control panel as the component's parent, except that currently it cannot get any callbacks from the component, which is kind of one-sided and not how react apps are normally built.

By allowing knobs to be controlled programmatically, the story can be isolated to just the component being demonstrated, leaving the actual state management mechanism opaque, as it should be for a simple presentational component like a checkbox. The checkbox itself doesn't care how it gets its props, it might get connected to redux, its parent might use setState, maybe it uses withState from recompose, or maybe the props are controlled by storybook knobs.

Anyways like I said I'm very new to this, so I'm only sharing my intuitive thoughts. If I'm off-base and there is a generally-accepted "best practice" for dealing with these kinds of pure stateless components, maybe someone can point me to a good example that I can follow?

@Yonben
Copy link

Yonben commented Jul 16, 2018

Hi :)

I just want to add I stumbled upon that as my components are receiving if they should display their mobile layouts (or not) from props, and my goal was to tie the viewport change to my knob, and it would have been really nice ;)

Just wanted to add my use case here, and as @IanVS even a callback would have been nice (so if I toggle my isMobile props, I could trigger a viewport change )

@aherriot
Copy link

I have heard several people express interest in a method to update knob state. If we can agree on a good architecture, I think this will be of value to many people. Especially since this is extra functionality that people can choose to use or not and because it doesn't negatively affect those who don't want to use it.

@igor-dv
Copy link
Member

igor-dv commented Jul 17, 2018

@Hypnosphi, you had objections about the previous implementation, WDYT?

@brianespinosa
Copy link

Commenting here to keep this issue open. I am still curious what @Hypnosphi has to say about @igor-dv previously proposed here.

@stale stale bot removed the inactive label Aug 7, 2018
@stale stale bot added the inactive label Aug 28, 2018
@Hypnosphi
Copy link
Member

It seems to me that the idea is not to introduce different sources of truth, but rather to allow maintaining a single source of truth for component state—the knobs. All of the other approaches I've seen suggested (wrapper components, state add-ons, recompose) would in fact introduce another source of truth. In my checkbox example, I would not be able to have a knob for checked, and also allow a wrapper component to provide the checked prop. I see the knobs control panel as the component's parent, except that currently it cannot get any callbacks from the component, which is kind of one-sided and not how react apps are normally built.

Sounds reasonable to me. Let's discuss the API

@stale stale bot removed the inactive label Sep 8, 2018
@brunoreis
Copy link

This will be a great feature. I just faced this same need here with a very basic controlled component and had faced it before in other components also. Thanks for looking at it.

To keep up with current syntax seems a little challenge. Maybe you could use something like:

const {value: name, change: setName} = text('Name', 'Kent');

@IanVS
Copy link
Member Author

IanVS commented Sep 14, 2018

@brunoreis, I would worry that changing the return signature of the knobs would be a breaking change. My off-the-cuff suggestion would be to do something like:

import {boolean, changeBoolean} from '@storybook/addon-knobs/react';

stories.add('custom checkbox', () => (
	<MyCheckbox
		checked={boolean('checked', false)}
		onChange={(isChecked) => changeBoolean('checked', isChecked)} />
));

Where the onChange callback might even allow currying, so that this would also work:

onChange={(isChecked) => changeBoolean('checked')(isChecked)}

// which of course simplifies down to
onChange={changeBoolean('checked')}

The important part is that the first argument must be the same as the label of the knob which should be changed. I believe this would allow users to opt-in to this behavior without changing anything about the way knobs are currently used. (Unless maybe labels are not currently required to be unique? I assume they are…)

@brunoreis
Copy link

@IanVS, that's nice. I agree with you about not changing the way things work. I could not find a way to do so because I did not think about using the label as a key. That might work. Let's see what @Hypnosphi has in mind.

@Hypnosphi
Copy link
Member

changing the return signature of the knobs would be a breaking change

Technically, that's not a problem right now. We have a major release upcoming. But it indeed would be better to allow some backwards compatibility.

I like the idea of currying support.

Unless maybe labels are not currently required to be unique?

Yes they are, and actually they are unique across different types. So there should be no need in separate change<Type> exports, just change should be enough. That's basically what was made in #3701
I think I'll just reopen that PR to let @aherriot finish it with some help from your side

@ndelangen
Copy link
Member

We can have this:

const {value: name, change: setName} = text('Name', 'Kent');

without having to change the return type of text.

Javascript functions are objects and thus can have properties.
Object can be destructured.

screen shot 2018-09-19 at 00 08 35

@Hypnosphi
Copy link
Member

Hypnosphi commented Sep 19, 2018

@ndelangen the text function is indeed an object, but its return value isn't. This won't work in your example:

const { foo, bar } = x() // note the parens

@Hypnosphi
Copy link
Member

We could have something like this though (the name is disputable):

const {value: name, change: setName} = text.mutable('Name', 'Kent');

@storybookjs storybookjs deleted a comment from stale bot Sep 23, 2018
@ndelangen
Copy link
Member

ndelangen commented Sep 23, 2018

why won't this work?

const { foo, bar } = x() // note the parens

The return of x is a function, which can have properties as well.

screen shot 2018-09-23 at 14 12 28

@Hypnosphi
Copy link
Member

Hypnosphi commented Sep 23, 2018

I was talking about x = () => {} from your original example.

If we make text return a function, users will need to change their code:

// Before
<Foo bar={text('Bar', '')}>

// After
<Foo bar={text('Bar', '')()}>
                         ^^ this

@ndelangen
Copy link
Member

I see

@stale stale bot added the inactive label Oct 15, 2018
@brunoreis
Copy link

What about @IanVS suggestion to this?

@stale stale bot removed the inactive label Oct 18, 2018
@storybookjs storybookjs deleted a comment from stale bot Oct 23, 2018
@ndelangen ndelangen added this to the next milestone Oct 23, 2018
@shilman shilman added this to Knobs in Hotlist Jan 16, 2020
@ldeveber
Copy link

ldeveber commented Mar 9, 2020

+1 for modals :)

@VladimirCores
Copy link

Can't believe that it's impossible from the beginning. Waiting for updates ...

@norbert-doofus
Copy link

Hi folks!
As a temporary solution I've used in my app next code:

import { addons } from '@storybook/addons';
import { CHANGE } from '@storybook/addon-knobs';

const channel = addons.getChannel();

channel.emit(CHANGE, {
  name: 'prop_name',
  value: prop_value,
});

Still waiting this feature will be implemented natively.

@tigredonorte
Copy link

tigredonorte commented May 6, 2020

I solved that problem using observable. I'm using storybook with angular

`
.add('updating chart data', () => {

const myObservable= new BehaviorSubject([{a: 'a', b: 'b'}]);
return {
  template: '
  <my-component
    myInput: myData,
    (myEvent)="myEventProp($event)"
  ></my-component>
  ',
  props: {
    myData: myObservable,
    myEventProp: $event => {
      myObservable.next([]);
      action('(myEvent)')($event);
    }
  }
};

})
`

@hsablonniere
Copy link

@norbert-doofus thanks your example helped me 👍

@shilman
Copy link
Member

shilman commented May 28, 2020

Hi gang, We’ve just released addon-controls in 6.0-beta!

Controls are portable, auto-generated knobs that are intended to replace addon-knobs long term.

Please upgrade and try them out today. Thanks for your help and support getting this stable for release!

@IanVS
Copy link
Member Author

IanVS commented May 28, 2020

Thats great! I can't quite tell from reading the README (I might have missed it), can these new controls be changed programmatically, which was the request being made in this issue?

@shilman shilman added this to the 6.0 args milestone May 28, 2020
@shilman
Copy link
Member

shilman commented May 28, 2020

They can be, although I'm not sure that API is officially supported yet (tho we are doing exactly that for the controls in addon-docs). I'll work with @tmeasday to figure out the best way to get that in after the first round of Controls bugs stabilize.

  • add updateArgs to the story context?
  • make the story context available to a callback that's called from the story? (this.context?.updateArgs(....))

@shilman
Copy link
Member

shilman commented Jun 1, 2020

For anybody who is interested in Controls but don't know where to start, I've created a quick & dirty step-by-step walkthrough to go from a fresh CRA project to a working demo. Check it out:

=> Storybook Controls w/ CRA & TypeScript

There are also some "knobs to controls" migration docs in the Controls README:

=> How do I migrate from addon-knobs?

@JCQuintas
Copy link

Hi folks!
As a temporary solution I've used in my app next code:

import { addons } from '@storybook/addons';
import { CHANGE } from '@storybook/addon-knobs';

const channel = addons.getChannel();

channel.emit(CHANGE, {
  name: 'prop_name',
  value: prop_value,
});

Still waiting this feature will be implemented natively.

Keep in mind that when using groupId you will need to add the group id to the name as follows:

 const show = boolean('Show Something', true, 'Group')

channel.emit(CHANGE, {
  name: 'Show Something_Group',
  value: prop_value,
});

Also, channel is an EventEmitter, so you can addListener to it to check what are the parameters being received.

channel.addListener(CHANGE, console.log)

@shilman
Copy link
Member

shilman commented Jul 17, 2020

Here's a code snippet for anybody who's interested in how to do this using addon-controls in v6.

import { useArgs } from '@storybook/client-api';

// Inside a story
export const Basic = ({ label, counter }) => {
    const [args, updateArgs] = useArgs();
    return <Button onClick={() => updateArgs({ counter: counter+1 })>{label}: {counter}<Button>;
}

I don't know if this is the best API for this purpose but it should work. Example in the monorepo:

https://github.com/storybookjs/storybook/blob/next/examples/official-storybook/stories/core/args.stories.js#L34-L43

@chrislopresto
Copy link

chrislopresto commented Jul 17, 2020

Thanks, @shilman! That did the trick.

For folks interested, we happen to have the exact Checkbox story checked prop that started this whole thread. Here's our newly wired up story using Storybook 6.0.0-rc.9:

export const checkbox = (args) => {
  const [{ checked }, updateArgs] = useArgs();
  const toggleChecked = () => updateArgs({ checked: !checked });
  return <Checkbox {...args} onChange={toggleChecked} />;
};
checkbox.args = {
  checked: false,
  label: 'hello checkbox!',
};
checkbox.argTypes = {
  checked: { control: 'boolean' },
};

cb-arg

@jcq
Copy link

jcq commented Jul 22, 2020

@shilman I attempted to utilize useArgs in a story for a controlled text input (a case where we might normally use the useState hook to update the value prop of the component via its onChange event). However, I encountered an issue where every time the user types a character, the component loses focus. Is this perhaps caused by it refreshing/re-rendering the story every time we update the args?

Is there a different recommended method for utilizing args/controls for a component with controlled text input?

This was with 6.0.0-rc.13

@shilman
Copy link
Member

shilman commented Jul 22, 2020

@jcq Can you create a new issue with a repro? This was not the primary use case for useArgs, but certainly one that we'd like to support, so we'd be happy to dig into it.

@jcq
Copy link

jcq commented Jul 23, 2020

@shilman No prob — here is the new issue:
#11657

I also should have clarified that the errant behavior shows in Docs, while it works correctly in the normal Canvas mode.

@shilman shilman modified the milestones: 6.0 args, 6.1 args Jul 30, 2020
@shilman shilman modified the milestones: 6.1 args, 6.2 args Oct 13, 2020
@ChazUK
Copy link

ChazUK commented Nov 10, 2020

I solved that problem using observable. I'm using storybook with angular

`
.add('updating chart data', () => {

const myObservable= new BehaviorSubject([{a: 'a', b: 'b'}]);
return {
  template: '
  <my-component
    myInput: myData,
    (myEvent)="myEventProp($event)"
  ></my-component>
  ',
  props: {
    myData: myObservable,
    myEventProp: $event => {
      myObservable.next([]);
      action('(myEvent)')($event);
    }
  }
};

})
`

This worked for me using Angular but changing to myData.value on line 5

@IanVS
Copy link
Member Author

IanVS commented Nov 10, 2020

I haven't tried this out yet (stuck on an older version of storybook for now) but it seems like this issue can be closed now. Thanks for the great work on args / controls!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Hotlist
Knobs
Development

No branches or pull requests