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

Add ability to selectively hide columns on ArgsTable #18928

Conversation

sir-captainmorgan21
Copy link

@sir-captainmorgan21 sir-captainmorgan21 commented Aug 13, 2022

Issue: #16179

What I did

  • Added a hideColumns prop to the ArgsTable component.
    • It allows for hiding the description, default, and control columns
  • Added stories for each column being hidden
  • Updated docs to include new prop

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?

@@ -87,7 +88,7 @@ export const ArgRow: FC<ArgRowProps> = (props) => {
<Name>{name}</Name>
{required ? <Required title="Required">*</Required> : null}
</StyledTd>
{compact ? null : (
{compact || hideColumns?.includes('descrption') ? null : (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work! Looks like there's a minor typo here:

Suggested change
{compact || hideColumns?.includes('descrption') ? null : (
{compact || hideColumns?.includes('description') ? null : (

Choose a reason for hiding this comment

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

good catch. I should probably check my story too. Haha feel like my story wouldn't have been working. Thanks!

Copy link
Contributor

@jonniebigodes jonniebigodes left a comment

Choose a reason for hiding this comment

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

@sir-captainmorgan21 this is really a cool feature, and thanks for working on this. From my end, I left a small note on some minor changes on the documentation side.

Let us know once you have had the chance to go over them so that we can follow up with you on this.

Hope you have a great week.

Stay safe

@@ -47,3 +47,5 @@ export interface Args {
}

export type Globals = { [name: string]: any };

export type HidableColumn = 'descrption' | 'default' | 'control';
Copy link
Contributor

Choose a reason for hiding this comment

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

@sir-captainmorgan21 as @MichaelArestad in the comment above, don't forget to update this one as well.

Choose a reason for hiding this comment

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

I suck. Thanks!

Comment on lines 51 to 55
| Option | Description |
| ------- | --------------------------------------------------------------------------------------------------- |
| `of` | Infers the args table from the component. <br/> `<ArgsTable of={MyComponent} />` |
| `story` | Infers the args table based on a story. <br/> `<ArgsTable story="example-mycomponent--my-story" />` |
| `hideColumns` | An array of columns to hide. Hidable Columns: `descrption`, `default`, `control`. <br/> `<ArgsTable of={MyComponent} hideColumns={['default', 'description']}` |
Copy link
Contributor

Choose a reason for hiding this comment

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

@sir-captainmorgan21, we have a bit of room for improvement here. If you wouldn't mind updating the table to the following:

| Option        | Description                                                                                                                                                                                |
| ------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| `of`          | Infers the args table from the component. <br/> `<ArgsTable of={MyComponent} />`                                                                                                           |
| `story`       | Infers the args table based on a story. <br/> `<ArgsTable story="example-mycomponent--my-story" />`                                                                                        |
| `hideColumns` | Toggles visibility for the args table columns.<br/>Available options: `description`, `default`, `control`. <br/> `<ArgsTable of={MyComponent} hideColumns={['default', 'description']} />` |

Also, if you are ok, could you create a small code snippet file in the docs/snippets/common directory with a small example showcasing the work you did and link below in the <CodeSnippets /> component? Turning it into:

<CodeSnippets
  paths={[
    'common/component-story-mdx-argstable-block-for-component.with-component.mdx.mdx',
    'common/component-story-mdx-argstable-block-for-story.with-story.mdx.mdx',
    'common/component-story-mdx-argstable-hide-columns.with-story.mdx.mdx',  //👈 Your snippet file here
  ]}
/>

With it, we can provide a bit more clarity on the documentation.

If you have trouble adding the snippets, this documentation provides you with the context you need, and if you have any questions regarding the documentation changes, feel free to ping me and I'll gladly answer them.

Choose a reason for hiding this comment

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

Ill knock these out. Thanks!

typos
docs clean up
@sir-captainmorgan21
Copy link
Author

@MichaelArestad @jonniebigodes thanks for the reviews. I addressed your comments. I did have a question:

Whats the best way to test and make sure the doc block works? I updated the component and wrote stories for the component. How can I make sure the doc block that uses the component works?

@MichaelArestad
Copy link
Contributor

MichaelArestad commented Aug 22, 2022

@sir-captainmorgan21 Thanks for making those changes! This looks good and works as expected.

You can find the published stories for ArgsTable here:
image

You can also run the Official Storybook locally via 'yarn start' in the code directory. You may need to run ./bootstrap.sh again to rebuild the Storybook UI.

We will look at this API change along side other changes considered for 7.0. We will leave this open and will follow up with an update.

@jonniebigodes
Copy link
Contributor

@sir-captainmorgan21 to get this pr moving forward; from my end, it looks good for the time being, as far as docs go. You'll probably need to check out the conflicts on this and act accordingly if you're willing. Also, @JReinhold, can you chime in on this as you've been doing some work on Doc Blocks? Is this something you've considered?

@JReinhold
Copy link
Contributor

On the surface this looks good @sir-captainmorgan21 !

However we're currently working on deprecating the ArgsTable block in favor of two separate blocks, Args (static table) and Controls (table with controls) in an attempt at making both the API and implementation simpler.

I could still see this feature in that setup though, so my suggestion would be that once we've finalised that work I'll ping you and you could then open a similar PR to those blocks. I suspect the implementation would be very similar to this.

Therefore I suggest we close this PR, as there's no real reason to add this nice feature to a soon-to-be-deprecated block.

@sir-captainmorgan21
Copy link
Author

@JReinhold oh ok awesome. Sounds good to me! Will you ping back here once the new Args and Controls tables are done? Or should I subscribe to another PR of this work?

@JReinhold
Copy link
Contributor

@sir-captainmorgan21 sorry I forgot this. They're done now! See them at #20664 and #20683

@JReinhold JReinhold closed this Jan 27, 2023
@sir-captainmorgan21
Copy link
Author

@JReinhold ahh ok sounds good. Ill open a new PR. Thanks!

@sir-captainmorgan21
Copy link
Author

@JReinhold I did have one question on those new components. I've upgraded to the latest SB 7.0 beta. However, the default values of the props aren't being set in my stories like before. I basically have to explicitly define all the prop values as opposed to letting the defaults do their thing. It is setting them to undefined if not provided

CC: @shilman since I know you do a good amount of work on Angular stuff and this seems related to compodoc

@JReinhold
Copy link
Contributor

@sir-captainmorgan21 could you open a new issue for that?

@sir-captainmorgan21
Copy link
Author

@JReinhold I can. FWIW, I did create a support post over in the Discord server

@sir-captainmorgan21
Copy link
Author

@JReinhold ok created a new issue: #21300 Thanks!

@vinodkola1337
Copy link

Any update on this feature? I am looking forward to use this in SB7.

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.

None yet

7 participants