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

InputControl: remove default value argument from Storybook #40410

Merged
merged 9 commits into from
Apr 21, 2022

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Apr 18, 2022

What?

Fixes a bug in InputControl's Storybook example, where currently it's not possible to interact with the component and enter a custom value

Why?

This is probably an unwanted side-effect introduced in #40119

How?

This PR removes value: '' from the list of default arguments for each Storybook example.

Testing Instructions

Open Storybook, make sure that it's possible to enter a custom value in the InputControl component and that the value is persisted after the input loses focus.

Make sure that any text manually set as the value via Storybook controls overrides every other text entered by the user.

Screenshots or screencast

Before:

input-control-value-before.mp4

After:

input-control-value-after.mp4

@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Type] Developer Documentation Documentation for developers [Package] Components /packages/components Storybook Storybook and its stories for components [Feature] Component System WordPress component system labels Apr 18, 2022
@ciampo ciampo requested a review from mirka April 18, 2022 12:00
@ciampo ciampo requested a review from ajitbohra as a code owner April 18, 2022 12:00
@ciampo ciampo self-assigned this Apr 18, 2022
@mirka
Copy link
Member

mirka commented Apr 19, 2022

Ah interesting!

The reason I added a value was so that the code snippets would show a stub value prop, which is one of the main props that the consumer will need to use.

I think the underlying tension is between wanting to showcase it simply as an uncontrolled component vs. showcasing it as a controlled component, the latter of which is how almost every consumer would use it.

I can think of two ways to somewhat reconcile this tension:

  1. Show the "complete" manual code snippet as defined in the main JSDoc comment a23ae2b. This is my preferred approach. It's low maintenance for us, and friendly for consumers. The stories themselves can remain simple (uncontrolled).

    Docs including the manual code snippet from the main JSDoc comment
  2. Switch stories to the controlled usage, and disable the Control for the controlled prop (as suggested in this article) 409b688. This would at least give us stubbed props for onChange and value in the code snippets, acting as a
    "minimum viable" code sample for the time being.

    Code snippet with stubbed props

What do you think?

@ciampo ciampo force-pushed the fix/components-input-control-story-value-interactivity branch from 409b688 to 4cc6042 Compare April 19, 2022 20:36
@ciampo
Copy link
Contributor Author

ciampo commented Apr 19, 2022

I also prefer option 1, but I also pushed a commit that introduces the "controlled" version as a separate story — maybe this could be the best compromise overall, what do you think?

I'm also happy to revert my changes and only include what we called "option 1"

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also prefer option 1, but I also pushed a commit that introduces the "controlled" version as a separate story — maybe this could be the best compromise overall, what do you think?

At the moment I don't have a strong opinion either way, but I'm curious what you think the purpose of the controlled story is. My worry is that it could be confusing to be there in the Docs view with no explanation, and with an "incomplete" autogenerated code snippet.

I noticed one benefit of having it, which is to have the use case be type checked. Like I noticed that this code sample in the JSDoc throws a type error for the onChange:

* const [ value, setValue ] = useState( '' );
*
* return (
* <InputControl
* value={ value }
* onChange={ ( nextValue ) => setValue( nextValue ) }
* />
* );

Though if it's just for type checking we could move it to a unit test.

@ciampo
Copy link
Contributor Author

ciampo commented Apr 20, 2022

My worry is that it could be confusing to be there in the Docs view with no explanation, and with an "incomplete" autogenerated code snippet.

You make a good point, let's not have a controlled story at least for now (it's always something that we can add later if we feel like we need it).

I went ahead and removed the controlled story, and then fixed the JSDoc snippets by adding a fallback empty string value in the onChange callback

@ciampo ciampo requested a review from mirka April 20, 2022 22:30
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with that! 🚢

Comment on lines 10 to 15
## 19.8.0 (2022-04-08)

### Bug Fix

- `InputControl`: allow user to input a value interactively in Storybook, by removing default value argument ([#40410](https://github.com/WordPress/gutenberg/pull/40410)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed it's under the wrong version heading 🧹

@ciampo ciampo merged commit 1b58a2b into trunk Apr 21, 2022
@ciampo ciampo deleted the fix/components-input-control-story-value-interactivity branch April 21, 2022 17:36
@github-actions github-actions bot added this to the Gutenberg 13.2 milestone Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components Storybook Storybook and its stories for components [Type] Bug An existing feature does not function as intended [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants