Add readonly option

[Vadorequest]

Issue: #14048

What I did

How to test

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

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

[shilman]

Looking great. Can you please add some examples to official-storybook? And some documentation to

https://github.com/storybookjs/storybook/blob/next/docs/essentials/controls.md

[Vadorequest]

The current implementation doesn't work on storybook-official story.

When I add a console.log in lib/components/src/controls/Text.tsx, it shows the expected value.

export const TextControl: FC<TextProps> = ({
  name,
  value,
  onChange,
  onFocus,
  onBlur,
  readOnly,
}) => {
  console.log('TextControl readOnly', readOnly) // Here, displays "true"
  const handleChange = (event: ChangeEvent<HTMLTextAreaElement>) => {
    onChange(event.target.value);
  };

But it only does so when clicking on the "Docs" tab, not on the "Canvas" tab.
I must be missing something.

[Vadorequest]

@shilman Documentation is done, could you look at the implementation and let me know if you notice anything wrong? (it's still not working)

[Vadorequest]

This doesn't look right to me, it fixes TS types in the various control option components, but I'm not sure it's correct.

Also, if it's correct, the disable property should be added there, too.

[shilman]

Sure, I'll take a look today!

[ghengeveld]

In light of #13890, shouldn't this property be one level up?

argTypes: {
  foo: {
    table: { disable: true },    // deprecated
    disable: true,               // the new way to disable (hide) an arg
    control: { readOnly: true }, // implemented in this PR
    readonly: true               // I suggest we do this instead
  }
}

I favor readonly over readOnly because of simplicity and being framework agnostic. readOnly seems like a React inspired prop name, but readonly (like the HTML attribute) is more common. In that regard I would've also preferred disabled over disable (or perhaps hidden).

[Vadorequest]

@ghengeveld I agree about the readonly naming, forgot storybook isn't react-specific.

Does the readonly option only apply to the control? Or could it be applied to other things? I don't really have an opinion whether readonly or control.readonly is better, but I wonder if it's an option that's specific to the control, or broader than that. Currently, my understanding is that it's specific to control and should be configured within it.

And, same goes for disable, I agree control.table.disable is too verbose, but I don't know if the disable option only disables it in the control, or if it's broader than that. And I think the final decision should be based on that.

[ghengeveld]

I had a discussion with @shilman about this, and we concluded that my earlier suggestion is the right way to go. For this PR, that means:

• readonly should be a property on the arg itself, not on control (i.e. one level higher).
• The readonly property should be respected by everything, not just Controls. That means it needs to be handled in the story store. Specifically, any readonly args need to be omitted from foundStory.args here (right after mapping/combining with the URL args):

  storybook/lib/client-api/src/story_store.ts

  Lines 241 to 248 in 4d50e9a

  	if (foundStory) {
  	if (args && foundStory.args) {
  	const mappedUrlArgs = mapArgsToTypes(args, foundStory.argTypes);
  	foundStory.args = combineArgs(foundStory.args, mappedUrlArgs);
  	}
  	this.setSelection({ storyId: foundStory.id, viewMode });
  	this._channel.emit(Events.STORY_SPECIFIED, { storyId: foundStory.id, viewMode });
  	}

  
  This should ensure that attempting to set a readonly arg through the args URL param will not be allowed. Note that we want to filter foundStory.args even if args (here, the URL args) are not present, because there are more ways to set args (e.g. on a story).

We'll deal with disable separately.

[Vadorequest]

I feel like I should probably wait for #13890 to be merged.

I'm glad TS types are being improved, it'll be easier to understand.

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[ndelangen]

@Vadorequest Thank you so soo much for this PR! We're really appreciative of any PR we get, because we know it takes a lot of time, effort and energy from contributors. This feature seems like something we want.

This has been in draft for over a year now, and the codebase has changed a lot since then, and with the upcoming changes for 7.0 that work would probably be even more complex.
Possibly not impossible, so please let me know if you're interested in re-opening this PR. But I'll close for now.

Again, we truly appreciate your time, energy and skill that you put into this PR! Thank you. I'm sorry for closing.

[Vadorequest]

Thanks for the heads-up - I had totally forgotten about this.

I won't work on it again, someone else should take the lead if you want this to be implemented :)

[LeaveNhA]

I understand this PR has closed, but have you ever thinking anything about this requirement?
It's a real-world requirement and I believe a lot of people might need it --like me, now--.

I will inspect the PR and if I wanna volunteer, I will inform you.

[MaliJetal]

Hi,
I really need this change will you please repopen this issue or create one more. If you want i can create a PR.
@ndelangen @LeaveNhA @Vadorequest