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

feat: add hook to find docgen props #1371

Merged
merged 3 commits into from
Jan 31, 2020
Merged

Conversation

mickaelzhang
Copy link
Contributor

@mickaelzhang mickaelzhang commented Jan 28, 2020

Description

In some cases, we want to have access to data that we have in docgen.
Some possible use case would be to filter some general props (similarly to https://sproutsocial.com/seeds/components/alert#properties with the system props) or if you inherit props from HTMLElement and you don't want it to show.

An example of useComponentProps's output would be:

// For a simple Button component for example:
const output = {
  variation: { 
    defaultValue: 'secondary', 
    description: "Variation of the Button", 
    name: 'variation', 
    parent: 'path/to/component.ts', 
    required: false, 
    type: "'primary' | 'secondary'", 
  },
  isDisabled: {
    defaultValue: 'false', 
    description: "Disable state", 
    name: 'isDisabled', 
    parent: 'path/to/component.ts', 
    required: false, 
    type: "boolean", 
  },
  ...
}

Related issue:

#895

Use case example

  • In Typescript, on a component we extends HTMLElement => too many props (100+ maybe?)
// path/to/HTMLElementComponent.ts

// Needed as docgen pick type from component and function only
export const HTMLElementComponent = (_: HTMLElement) => null
// gatsby-theme-docz/components/Props.js

export const Props = ({ props, ...others }) => {
  const htmlElementProps = useComponentProps({ 
    componentName: 'HTMLElementComponent', 
    fileName: 'path/to/HTMLElementComponent.ts' 
  })
  const displayedProps = filterPropsLogic(props, htmlElementProps)

  return <DoczProps props={displayedProps} {...others} />
}
  • We want to remove a multiple group of component from all components
// path/to/SystemProps.ts

// Needed as docgen pick type from component and function only
export const MarginComponent = (_: MarginProps) => null
export const PaddingComponent = (_: PaddingProps) => null
// gatsby-theme-docz/components/Props.js

export const Props = ({ props, ...others }) => {
  const marginProps = useComponentProps({ 
    componentName: 'MarginProps', 
    fileName: 'path/to/SystemProps.tss' 
  })
  const paddingProps = useComponentProps({ 
    componentName: 'PaddingProps', 
    fileName: 'path/to/SystemProps.tss' 
  })
  const displayedProps = filterPropsLogic(props, [marginProps, paddingProps])

  return <DoczProps props={displayedProps} {...others} />
}

Subject to change if it doesn't fit all use cases.

@mickaelzhang mickaelzhang marked this pull request as ready for review January 29, 2020 10:30
@mickaelzhang
Copy link
Contributor Author

Hello @droganov @maryiam!
I've been working on this and wanted get your feedback since you commented on the following issue #895

Does it fit your use case as well?

@droganov
Copy link

@mickaelzhang great, looks promising.
is there useComponentProps output example?

One more idea would be to patch output:

<DoczProps>
  {children => children.reduce(whatever)}
</DoczProps>

in case you can inject metadata to children

@maryiam
Copy link

maryiam commented Jan 29, 2020

Thanks !! It would be great indeed to have an example of output!

@mickaelzhang
Copy link
Contributor Author

mickaelzhang commented Jan 29, 2020

@droganov The output of useComponentProps is the same as the one you receive from the prop props of <Props /> when you shadow it. So something like that:

{
  propName: { 
    defaultValue: null, 
    description: 'Prop name description', 
    name: 'propName', 
    parent: 'path/to/component.ts', 
    required: true, 
    type: 'string', 
  },
  anotherPropName: { ... },
  ...
}

I edited the description of the PR with an example of output

I'm not sure that understand your example 😅 If it's about the example that I give, you could completely change the logic if you want, the example that I give is the shadowed component so you have the complete control over that!

@aarondancer @ejuo @chunksnbits I just saw that you guys also upvoted the original issue, do you guys have an opinion on this?

@ejuo
Copy link
Contributor

ejuo commented Jan 29, 2020

@mickaelzhang
For me, the main problem is the inheritance of system properties. Docz filters them by default. But updates periodically break it. In typescript 3.6 it fixed, but in 3.7 breaks again.
But as the additional option, perhaps it would be convenient.

@droganov
Copy link

@mickaelzhang I thought in case it outputs N rows for N props, we could inject corresponding prop to each row as static member.

Then we could iterate children knowing component name, displayName and path, and create local component that abstract that problem away.

We still could do so with the hook I think.

@mickaelzhang
Copy link
Contributor Author

For me, the main problem is the inheritance of system properties.

@ejuo I see, so your problem is similar to @maryiam

@droganov Ok, I still don't understand sorry 😂 Maybe a clear example of what you want to do would help

@droganov
Copy link

droganov commented Jan 29, 2020

@mickaelzhang

You change Props component to accept function as a child:

({ of, children }) => {
  const result = Object.entries(of).map(renderPropAndInjectMetadata);
  return typeof children === 'function' ? children(result) : result; 
}

Then we can filter rendered rows without hooking any paths manually:

<Props of={UiComponent}>
  {renderedProps => children.filter(propRow => propRow.componentName === 'UiComponent')}
</Props>

I mean when Props component knows paths it looks like a sort of overkill, to pass them as strings to the hook.

@mickaelzhang
Copy link
Contributor Author

@droganov Ah yes, I understand now! So you have a case where you want to filter in the mdx file directly, per case, right?

This is a bit different than the case describe in my PR then, what I'm trying to solve is having a way to filter on all component at once with props definitions from other components.

What you are describing is possible right now by shadowing the <Props /> component already btw

@mickaelzhang
Copy link
Contributor Author

@droganov The use case that you describe is interesting. Maybe you could open an issue so we can discuss about this specific use case and if it should be a built-in feature?

For this PR in particular, it seems like it look good to all of you? We can move to the review part then!

Copy link
Contributor

@rakannimer rakannimer left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

@rakannimer
Copy link
Contributor

It would also be cool if someone could add a small example to the examples directory to help others quickly know how to filter props. 👍

@rakannimer rakannimer merged commit 8fffa26 into doczjs:master Jan 31, 2020
@mickaelzhang mickaelzhang deleted the props-hook branch January 31, 2020 15:26
@maryiam
Copy link

maryiam commented Jan 31, 2020

Can you please add to the documentation the explanation on how to filter props? Thanks 🙏

@mickaelzhang
Copy link
Contributor Author

@maryiam I will when I can find a bit of time! :)

@maryiam
Copy link

maryiam commented Feb 3, 2020

Ok thank you @mickaelzhang. Also, it would be great to mention the released version on which this feature will be available. 🙌

@mickaelzhang
Copy link
Contributor Author

@maryiam PR opened on the documentation, doczjs/docz-website#98

Let me know if you have any suggestion to make it clearer 👍

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

Successfully merging this pull request may close these issues.

5 participants