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

Addon-docs: Add controls to ArgsTable #10354

Merged
merged 35 commits into from
Apr 15, 2020
Merged

Addon-docs: Add controls to ArgsTable #10354

merged 35 commits into from
Apr 15, 2020

Conversation

shilman
Copy link
Member

@shilman shilman commented Apr 9, 2020

Issue: #6639

What I did

This is the second of a series of PRs to add controls (formerly known as "knobs") to the Props block:

  • Convert Props to use ArgsTable (this PR)
  • Add Controls to ArgsTable
  • Change parameterEnhancers to a more restrictive ArgTypes enhancer
  • Performance improvements

This is a telescoping PR that targets the first step, and will solve a few of the outstanding issues from that PR. It's separated to make it easier to review.

Open questions

  • Currently we use the 'other' type to capture any type we don't know about it, and show an Object control. I'm wondering whether we need to have an 'unknown' type (???) where we show no control at all. We could use this for React types, for example.
  • Typescript intersection/union/interfaces handling

Key files to review

  • enhanceArgTypes.ts / .test.ts - logic for how it all comes together
  • convert.test.ts - type conversion logic
  • ArgControl.tsx - control component for the table
  • props.stories.mdx - e2e tests

How to test

  • Addons/Docs/props stories e2e stories

@shilman shilman changed the base branch from next to 6639-args-table April 9, 2020 05:45
@shilman shilman changed the title 6639 args table controls Addon-docs: Add controls to ArgsTable Apr 9, 2020
@shilman shilman marked this pull request as ready for review April 9, 2020 23:50
@shilman shilman changed the base branch from 6639-args-table to next April 14, 2020 06:55
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Looks great!

`);
});

it('extracted argTypes take precedence over inferred argTypes', () => {
Copy link
Member

Choose a reason for hiding this comment

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

🤔 are we sure about this?

`);
});

it('user-specified argTypes take precedence over inferred argTypes', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder about this too.. I guess it'll come out in the wash when the rubber hits the road...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants