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

[Docs] Allow passing a prop type or interface directly to the props option in the docs #1688

Closed
cchaos opened this issue Mar 6, 2019 · 15 comments · Fixed by #3771
Closed
Assignees
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries feature request

Comments

@cchaos
Copy link
Contributor

cchaos commented Mar 6, 2019

I have a shared TS interface definition that I used throughout a component and it's sub-components. I'd like to display this interface and any prop comments in the props table.

An example of this type of definition is

export type EuiPopoverPosition = 'top' | 'right' | 'bottom' | 'left';

which lives in it's own file.

I'd like to be able to do something like

import { EuiPopoverPosition } from '../../.....'

{
  props: { EuiPopoverPosition }
}

in the docs.

@cchaos cchaos added documentation Issues or PRs that only affect documentation - will not need changelog entries assign:engineer feature request labels Mar 6, 2019
@cchaos cchaos mentioned this issue Mar 11, 2019
8 tasks
@cchaos cchaos added the :urgent: label May 7, 2019
@cchaos
Copy link
Contributor Author

cchaos commented May 7, 2019

With all the conversions of typescript allowing us to create re-usable prop types, this is becoming more necessary. I'd like to bump the urgency of it. Otherwise I've been needing to write out the prop definitions directly in the Doc text.

@thompsongl
Copy link
Contributor

thompsongl commented Oct 3, 2019

Finally looking into this.

Given a props table like

Screen Shot 2019-10-03 at 3 24 35 PM

where display is a prop that uses a type much like you're describing
(type EuiColorPickerDisplay = 'default' | 'inline')
I'm trying to understand where this additional type/interface display comes in. If the type is in use, it'll display the usage ('type' column) in the relevant prop context. If the type is not used by any props in the table, why does it need to be there?

I could see a case for adding a column to the existing table to show the name of the type/interface that the prop uses. Otherwise, the inserted type probably deserves it own table as the columns wouldn't really match the existing, but it still seems to lack context.

@thompsongl
Copy link
Contributor

@chandlerprall
Copy link
Contributor

"interesting workaround" may be too much credit. Would still prefer finding a more 'official' way, but it's probably something that transpiles to that structure.

@cchaos
Copy link
Contributor Author

cchaos commented Oct 28, 2019

Yeah, the workaround is working for me but would like a more automatic way so we don't have to create the special props files.

@thompsongl
Copy link
Contributor

@ashikmeerankutty Re: #3056 (comment)

Having the "base" component type extend additional types and interfaces doesn't quite get what we need. We need the additional type to be more standalone and include all of its props regardless if other types have props of the same name.

@chandlerprall had a workaround mentioned earlier in this thread, and noted:

Would still prefer finding a more 'official' way, but it's probably something that transpiles to that structure.

This may be worth more exploration.

@thompsongl
Copy link
Contributor

@ashikmeerankutty We have people on vacation, etc., so it might be a couple days before your main PR gets merged. In meantime, let's focus on this work if you haven't started yet.

@ashikmeerankutty
Copy link
Contributor

@thompsongl Typescript strips all the types and interfaces. So the only way I think of is to transform those types or interface to some objects. And append it to AST. I don't know if that is possible. Is there any possibility for that ?

@thompsongl
Copy link
Contributor

Yeah, I think a transformation of some kind is in line with our thinking. That is how the current workaround is done: https://github.com/chandlerprall/eui/blob/6183abfbc1e3268a63375ae91aa0c7b739043d50/src-docs/src/views/datagrid/props.tsx

Basically the type/interface gets converted to a component so the parser picks it up. We'd just need it to be more automated.

@ashikmeerankutty
Copy link
Contributor

I can get the interfaces and types from typescript. So create a component with that interface automatically whenever an interface is found Sound ok. I will check that out.

@ashikmeerankutty
Copy link
Contributor

ashikmeerankutty commented Jul 19, 2020

I found a solution for this issue. Its another babel plugin that finds all the interfaces and types in a file and converts it to an object with ___docgen property and append it to the AST.

So we can access props info by just passing the interface to guideSectionPage

It will create results like this for interfaces.

image

In case of types I have no idea how the result must looks like.

This is a follow up for #3554. So should I commit changes to that branch itself ?

@ashikmeerankutty
Copy link
Contributor

For the above example of type. I was able to generate doc info as following:

{
  "name": "EuiPopoverPosition",
  "__docgenInfo": {
    "displayName": "EuiPopoverPosition",
    "props": {
      "EuiPopoverPosition": {
        "name": "EuiPopoverPosition",
        "type": {
          "name": "\"top\",\"right\",\"bottom\",\"left\""
        }
      }
    }
  }
}

And this can be used as stated in the issue. I can change the result into any structure.

@thompsongl
Copy link
Contributor

Great! Thanks for exploring this. What plugin is used to get this result?

This is a follow up for #3554. So should I commit changes to that branch itself ?

Let's make this its own PR. If necessary, have the PR point to the 3554 branch as the merge target for now.

@ashikmeerankutty
Copy link
Contributor

ashikmeerankutty commented Jul 20, 2020

Let's make this its own PR. If necessary, have the PR point to the 3554 branch as the merge target for now.

Its a custom plugin. I will make a separate PR

@thompsongl
Copy link
Contributor

Its a custom plugin.

Nice! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants