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

Allow passing a prop type or interface directly to the props option in the docs #3771

Merged
merged 18 commits into from
Aug 11, 2020

Conversation

ashikmeerankutty
Copy link
Contributor

@ashikmeerankutty ashikmeerankutty commented Jul 20, 2020

Summary

Fixes #1688

  • Added a custom webpack loader to transform types and interfaces to objects and export them from the files.
  • Added typescript module declaration for the custom loader
  • Property __docgenInfo of the object contains types of props so it can be directly used in the docs page.
  • Added interfaces and types of Datagrid and ColorPicker examples to docs.
  • Used resolveLoader in webpack config to create alias for relative path of loader.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation
- [ ] Checked Code Sandbox works for the any docs examples
- [ ] Added or updated jest tests

  • Checked for breaking changes and labeled appropriately

- [ ] Checked for accessibility including keyboard-only and screenreader modes
- [ ] A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@thompsongl
Copy link
Contributor

Can you add examples to show this working? The EuiPopoverPosition and PaginationBarProps output shown in the issue would be good enough, I think.

@ashikmeerankutty
Copy link
Contributor Author

Can you add examples to show this working? The EuiPopoverPosition and PaginationBarProps output shown in the issue would be good enough, I think.

Added a basic example. it could be found on the props table of basic table.

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3771/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Nice, that didn't end up too bad :)

For the EuiPopoverPosition & PaginationBarProps examples, those should be applied to their respective pagees/sections instead of taking over the basic table's props rendering.

@ashikmeerankutty
Copy link
Contributor Author

For the EuiPopoverPosition & PaginationBarProps examples, those should be applied to their respective pagees/sections instead of taking over the basic table's props rendering.

I added those for demo 😄 . I guess there are so many possible places where props of types and interfaces must be shown. Should I do that ?

@chandlerprall
Copy link
Contributor

I added those for demo 😄 . I guess there are so many possible places where props of types and interfaces must be shown. Should I do that ?

Gotcha! Let's get at least a couple of those done as part of this PR. The other instances can follow later, especially if you find yourself waiting on us for something else.

@ashikmeerankutty
Copy link
Contributor Author

EuiPopOverPosition is not used in any of the components as a prop type. PaginationBarProps is used in basic table. but the doc info of basic table is added manually. Should I replace PaginationBarProps in the doc info of basic table ?

@chandlerprall
Copy link
Contributor

Should I replace PaginationBarProps in the doc info of basic table ?

Nah; let's see, I think the data grid props would really benefit from this

props: {
EuiDataGrid,
EuiDataGridColumn: DataGridColumn,
EuiDataGridColumnVisibility: DataGridColumnVisibility,
EuiDataGridControlColumn: DataGridControlColumn,
EuiDataGridInMemory: DataGridInMemory,
EuiDataGridPagination: DataGridPagination,
EuiDataGridSorting: DataGridSorting,
EuiDataGridCellValueElement: DataGridCellValueElement,
EuiDataGridSchemaDetector: DataGridSchemaDetector,
EuiDataGridStyle: DataGridStyle,
EuiDataGridToolbarVisibilityOptions: DataGridToolbarVisibilityOptions,
EuiDataGridToolBarVisibilityColumnSelectorOptions: DataGridToolBarVisibilityColumnSelectorOptions,
EuiDataGridPopoverContent: DataGridPopoverContent,
},

Another good place is the color picker.

@ashikmeerankutty
Copy link
Contributor Author

When using the new method in data grid it was found that types are not expanded For eg:

image

I think this can be fixed in the plugin. I will try to fix this and commit.

@ashikmeerankutty
Copy link
Contributor Author

One thing to note here. If the types are exported using index.ts. It will show as the type is not exported. Since typescript strips that information. In order to work the types must be exported directly from the file

@chandlerprall
Copy link
Contributor

The props tab for Color palette picker errors with

guide_section.js:30 Uncaught TypeError: Cannot read property 'split' of undefined
at markup (guide_section.js:30)
at guide_section.js:316
at Array.map ()
at GuideSection.renderPropsForComponent (guide_section.js:292)
at guide_section.js:412
at Array.map ()
at GuideSection.renderProps (guide_section.js:411)
at GuideSection.renderContent (guide_section.js:495)
at GuideSection.render (guide_section.js:522)
at finishClassComponent (react-dom.development.js:18470)

@ashikmeerankutty
Copy link
Contributor Author

The props tab for Color palette picker errors with

guide_section.js:30 Uncaught TypeError: Cannot read property 'split' of undefined
at markup (guide_section.js:30)
at guide_section.js:316
at Array.map ()
at GuideSection.renderPropsForComponent (guide_section.js:292)
at guide_section.js:412
at Array.map ()
at GuideSection.renderProps (guide_section.js:411)
at GuideSection.renderContent (guide_section.js:495)
at GuideSection.render (guide_section.js:522)
at finishClassComponent (react-dom.development.js:18470)

Cache showed no error so I missed it 😞 . I will fix this.

@ashikmeerankutty
Copy link
Contributor Author

@chandlerprall Thanks for the review. Fixed it 😃

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3771/

@chandlerprall
Copy link
Contributor

chandlerprall commented Jul 27, 2020

Thinking through this change and how it can impact things in the future: the syntax of passing a type/interface as a value in the props object is invalid typescript, and when the examples are converted will need a lot of @ts-ignore to make it work.

Only thing I can think of is, instead of making this a babel plugin, do this same logic/conversion in a webpack loader. Then we can specify the loader in the examples when importing the type,

import * as EuiColorProps from '!!props-loader!../../../../src/components/color_picker/color_palette_picker/color_palette_picker';

...

{props: pick(EuiColorProps, ['EuiColorPalettePickerPaletteText', 'EuiColorPalettePickerPaletteFixed', 'EuiColorPalettePickerPaletteGradient'])}

Webpack loader results can be defined in .d.ts files, like

declare module '!!props-loader!*' {
  const contents: { [key: string]: PropsDefinitionType };
  export = contents;
}

@ashikmeerankutty
Copy link
Contributor Author

Only thing I can think of is, instead of making this a babel plugin, do this same logic/conversion in a webpack loader.

Sure I will convert this into a webpack loader

@chandlerprall
Copy link
Contributor

@ashikmeerankutty I'm looking into how to replace the type/interface with the object, instead of appending. I have that "working", but for some reason babel still sees/errors on a duplicate export name 😕

I'll let you know what I find

…types & interfaces; use the loader specifically when needed
@chandlerprall
Copy link
Contributor

chandlerprall commented Aug 3, 2020

@ashikmeerankutty I sent a PR your way ashikmeerankutty#4

I don't like having to use a relative path to specify the prop-loader , would you mind investigating some to see if there's a way to specify a local webpack loader without the relative path? I have some ideas for "fixing" that, but I'd like to see if anyone has a better solution first :)

note: I have not addressed the typescript module issue, as the current examples aren't TS files. Also think we need to address the loader-as-a-relative-path issue before the TS module.

@ashikmeerankutty
Copy link
Contributor Author

@chandlerprall Reading through the docs https://webpack.js.org/configuration/resolve/#resolvealias this seems to work. But I couldn't make it work on the project. 😞

@ashikmeerankutty
Copy link
Contributor Author

@chandlerprall Reading through the docs https://webpack.js.org/configuration/resolve/#resolvealias this seems to work. But I couldn't make it work on the project. 😞

Fixed it. Can you please review it

@chandlerprall
Copy link
Contributor

chandlerprall commented Aug 5, 2020

Nice, good find with resolveLoader!! src-docs/src/views/datagrid/props.tsx and src-docs/src/views/color_picker/props.tsx should be deletable now. I expect the playground things you commented out can be re-enabled now too.

I'm looking into the typescript side of this; I can get a module declaration for the loader, but having some issues with getting the imports right, mostly around being able to define arbitrary named exports.

@cchaos would you mind taking a look at the usage here? Primarily the !!prop-loader!... imports in the data grid and color picker example files. Just want a sanity check that it isn't a terrible change in developer experience.

@ashikmeerankutty
Copy link
Contributor Author

I'm looking into the typescript side of this; I can get a module declaration for the loader, but having some issues with getting the imports right, mostly around being able to define arbitrary named exports.

I tried using the wildcard but still its showing cannot find module

@cchaos
Copy link
Contributor

cchaos commented Aug 5, 2020

I think that's ok from a dev/syntax experience. So long as we have examples to copy/paste from and probably add a section to our Documentation guidelines wiki doc.

Is this PR also supposed to show the examples currently working in the docs? I'm wondering how we will now "link" to the props table as references like how we would do #EuiPropsOfSomething in the prop comments.

@chandlerprall
Copy link
Contributor

I tried using the wildcard but still its showing cannot find module

I added a prop-loader.d.ts file into src/custom_typings with

declare module '!!prop-loader!*' {
  const contents: Record<string, any>;
  export = contents;
}

which allows, e.g.

import * as EuiDataGridTypes from '!!prop-loader!...';

but not

import {TypeA, TypeB, TypeC, ...} from '!!prop-loader!...';

@chandlerprall
Copy link
Contributor

I think that's ok from a dev/syntax experience. So long as we have examples to copy/paste from and probably add a section to our Documentation guidelines wiki doc.

Is this PR also supposed to show the examples currently working in the docs? I'm wondering how we will now "link" to the props table as references like how we would do #EuiPropsOfSomething in the prop comments.

The links continue working the same, just verified locally where EuiDataGridColumnVisibility references EuiDataGridColumn and that jump works.

@ashikmeerankutty
Copy link
Contributor Author

import {TypeA, TypeB, TypeC, ...} from '!!prop-loader!...

I couldn't find a better way to solve this issue till now. The only way I found was to define the module as any as

 declare module '!!props-loader!*';

so that we can import in any way we want.

This can avoid the use of tsignore.

@chandlerprall
Copy link
Contributor

chandlerprall commented Aug 6, 2020

import {TypeA, TypeB, TypeC, ...} from '!!prop-loader!...

I couldn't find a better way to solve this issue till now. The only way I found was to define the module as any as

 declare module '!!props-loader!*';

so that we can import in any way we want.

This can avoid the use of tsignore.

Perfect! I confirmed that module declaration allows the import. Woo! Can you push that .d.ts file and update the PR description to reflect all the changes? I don't think this needs any work beyond that; I'll do a another review after this last addition is pushed.

@ashikmeerankutty
Copy link
Contributor Author

ashikmeerankutty commented Aug 6, 2020

Can you push that .d.ts file

Should I add the declaration file to src/custom_typings directory or in the scripts directory ?

@chandlerprall
Copy link
Contributor

Please add it to src/custom_typings, typescript is already configured to look there and will pick it up automatically.

@ashikmeerankutty
Copy link
Contributor Author

ashikmeerankutty commented Aug 6, 2020

@chandlerprall Pushed the changes 😃 . Can you please review it

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3771/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM! Examples' props come through the same, new code looks good, nice isolated use with the custom loader.

Thanks again @ashikmeerankutty !

@chandlerprall chandlerprall merged commit 7572226 into elastic:master Aug 11, 2020
@thompsongl
Copy link
Contributor

🎉 Great work!

nyurik pushed a commit that referenced this pull request Aug 18, 2020
… in the docs (#3771)

* Add docgen from ts  props  babel plugin

* Add a basic example for demo

* Removed example

* Remove props.tsx and use props directly

* Expand types

* Passed interfaces directly to props in color picker example

* Fixed type value in extended props

* Added props-loader to convert interfaces to props

* removed logger for result

* Add comments

* Update the prop-loader script to output only the processed, exported types & interfaces; use the loader specifically when needed

* Fixed relative path for loader

* Removed props.tsx files and uncomment playground files

* removed docgen from ts props

* Declare module as any

* Add prop-loader declaration to custom typings

Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
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.

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