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

[Doc] Autogenerate the property description fo the AppBar #2382

Merged
merged 3 commits into from
Dec 7, 2015
Merged

[Doc] Autogenerate the property description fo the AppBar #2382

merged 3 commits into from
Dec 7, 2015

Conversation

oliviertassinari
Copy link
Member

@subjectix Are you ok with this way? It's like what react-native is doing https://github.com/facebook/react-native/blob/master/Libraries/Components/SwitchIOS/SwitchIOS.ios.js#L46.

@alitaheri
Copy link
Member

@oliviertassinari I think we should provide some meta data for our tooling to better render the final html. First we must use Markdown, second we could add some @ tags (depending on tooling) to provide automation hooks. But for now it's awesome. just consider using markdown where ever needed.

@oliviertassinari
Copy link
Member Author

What's the markdown feature for?
I plan to use https://github.com/reactjs/react-docgen.

@oliviertassinari
Copy link
Member Author

But yes, we could convert the output to markdown with https://github.com/reactjs/react-docgen/blob/master/example/generateMarkdown.js and then to HTML with #2226

@alitaheri
Copy link
Member

I plan to use https://github.com/reactjs/react-docgen.

Good decision.

What's the markdown feature for?

For code examples, acceptable value tables, etc...

@oliviertassinari
Copy link
Member Author

code examples

I don't think that code examples should be in the description.
For this, I plan to create separe component. We could then use each example component for three use case:
-testing
-demonstration
-code description

acceptable value tables

This will be extracted from the source code

@alitaheri
Copy link
Member

@oliviertassinari Well you make some good points. Alright, I'm ok with this. 👍 👍

@oliviertassinari
Copy link
Member Author

@subjectix @shaurya947 I think that I'm done for this first iteration. Here is the result:
screen shot 2015-12-06 at 01 27 53

The doc build fails, I'm waiting benjamn/recast#238 to be released.

@oliviertassinari oliviertassinari mentioned this pull request Dec 6, 2015
@alitaheri
Copy link
Member

@oliviertassinari Good job 👍 Is there anyway to move the example and description to the source code too? Something like, jsdoc on the class deceleration?

@oliviertassinari
Copy link
Member Author

I'm done. @subjectix can you review?
screen shot 2015-12-06 at 13 11 59

That the new look of the documentation page https://github.com/oliviertassinari/material-ui/blob/doc-autogenerated/docs/src/app/components/pages/components/app-bar.jsx.

@alitaheri
Copy link
Member

😱 Give me some time 😄

@@ -0,0 +1,31 @@
import React from 'react';
import AppBar from 'material-ui/app-bar';
Copy link
Member Author

Choose a reason for hiding this comment

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

/lib is missing, I don't know how to make it work.

Copy link
Member

Choose a reason for hiding this comment

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

try running npm run build -- --watch on the root of the project to see if it helps.

can't webpack map paths?

@alitaheri
Copy link
Member

I can't get it to work:

Webpack outputs this:

ERROR in ./~/recast/main.js
Module not found: Error: Cannot resolve module 'fs'

@oliviertassinari
Copy link
Member Author

I need benjamn/recast#238 to be released.

@alitaheri
Copy link
Member

Nevermind I cloned into the recast repo. It's fixed for now.

@alitaheri
Copy link
Member

Ok I saw how it turned out. There are 2 major issues,

  1. material-ui/lib/... must be fixed somehow.
  2. props should be categorized, structured and be more readable. right now it's hard to distinguish each prop.

and some other prettier patterns:

  1. lambda doesn't need explicit return.
  2. AppBarExampleIconButton better be a class, the external function may confuse people.

P.S. stop rebasing, I can't track changes >.>

@oliviertassinari
Copy link
Member Author

  1. I can't agree more!
  2. I'm following react-native style (http://facebook.github.io/react-native/docs/mapview.html#content)
    How would you improve it?
  3. I guess I should use React.createClass for the examples.

stop rebasing, I can't track changes

Sorry

@alitaheri
Copy link
Member

IMO a table can be more pleasing to the eye. like how react-bootstrap does it. I mean even reading the props from react-native hurts my eyes 😢

If you want to keep it this way at least make the names bold. use mono-space for type and defaults, and put more space between each prop.

asides from these, really awesome job 👍 👍 👍

@oliviertassinari
Copy link
Member Author

IMO a table can be more pleasing to the eye

Let's try it.

@oliviertassinari
Copy link
Member Author

That's better 👍

screen shot 2015-12-06 at 15 19 34

screen shot 2015-12-06 at 15 09 12

@alitaheri
Copy link
Member

@oliviertassinari It's beautiful. Great job 👍 👍 👍

@alitaheri
Copy link
Member

@oliviertassinari I think it's better to annotate only the required props and oit all the optional annotations. isn't that better? since only so few are required.

@frankf-cgn
Copy link

@oliviertassinari You letting generate markdown within the PropTypeDescription is pretty awesome - nice work! 👍
After renaming from "Description" to "PropTypeDescription" was it intentional to let the const remain to "Description" (src/app/components/PropTypeDescription.jsx#L19)?

@oliviertassinari
Copy link
Member Author

After renaming from "Description" to "PropTypeDescription" was it intentional to let the const remain to "Description"

Oups, thanks for finding this.

@oliviertassinari
Copy link
Member Author

I have taken into account your feedbacks.

It is used for navigation, search branding, and actions.
An app bar is also referred to as the primary toolbar or action bar for Android.

### Examples
Copy link
Member

Choose a reason for hiding this comment

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

Should this be here? I mean... shouln't it be where this file is read from?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get it. What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Examples is inside the readme file, that means every read me MUST end with ###Examples. shouldn't it be where you make the final markdown FROM this readme file and append this there? also maybe the title should be there too.

Copy link
Member

Choose a reason for hiding this comment

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

I think the best practice would be to add a HeaderSection component that takes a title and a description markdown text and renders the header section. just like the PropTypeDescription,

@alitaheri
Copy link
Member

@oliviertassinari Also, can you please use markdown in props description too? use ` for references and codes.
Very awesome job 👍 👍

@@ -0,0 +1,7 @@
## AppBar
Copy link
Member

Choose a reason for hiding this comment

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

This too maybe?

@frankf-cgn
Copy link

@subjectix Don't you think that gets too specialized? I can easily reason about the component <PropTypeDescription>. It takes burden from the developer because the doc gets autogenerated. Awesome. But by introducing another component like <HeaderSection> we are definetly going to build an abstraction onto an abstraction.
Let's return more to kiss instead of throwing lot's of components onto this.
@oliviertassinari Just an rough idea: Maybe it is possible to inject the html the component <CodeExample> generates into <MarkdownElement>?

@alitaheri
Copy link
Member

@frankf-cgn you have a point, but that ###Example should go away don't you think?

@frankf-cgn
Copy link

@subjectix Indeed you have a point, too. The need to have a heading of a following component in the markdown-file makes it though to understand it. So maybe the idea with injecting the <CodeExample> into the <MarkdownElements>might be a solution, maybe @oliviertassinari has a better idea.
Let me sleep about this.

@alitaheri
Copy link
Member

@frankf-cgn I like you idea. have a good night 💤

@oliviertassinari
Copy link
Member Author

we are definetly going to build an abstraction onto an abstraction.

I think that the best abstraction for the doc is the markdown format.
Having say that, I can't see any good way to improve the current solution.

@frankf-cgn
Copy link

Sadly, my "injecting"-idea does not work. That's because <CodeExample> not only renders html but the working component, too. And that is really cool, because the doc and the example literally share the same code!
Compared to the documentation effort of other projects, what @oliviertassinari created with the help of <PropTypeDescription> and the restructuring of the documentation-folder layout is more advanced, powerfull and easy to approach.

So my 2 cents: Lets go ahead with this PR.
@oliviertassinari: Do you need any help on this? Do you want to switch the whole documentation to the new system at once within this PR? If we instead go on with one [Doc Enhamcement]-PR per component we can share the work ;-)

@oliviertassinari
Copy link
Member Author

So my 2 cents: Lets go ahead with this PR

🎉

I want to merge this PR first, but I need benjamn/recast#238 to be release 😒. Or to find a way to fix it here.

Do you need any help on this.

I think that we should migrate component per component. So yes 👍.

@frankf-cgn
Copy link

@oliviertassinari Did you like this styling more than the default gfm-table?
If yes, I will invest some time to make it work.
bildschirmfoto 2015-12-07 um 14 44 28

@alitaheri
Copy link
Member

@frankf-cgn Lovely 👍

@oliviertassinari
Copy link
Member Author

@frankf-cgn Their is still room for improvement 👍.
I have added a temporarty fix for recast. I'm gonna merge this.

@subjectix @frankf-cgn Thanks for your help 🎉.

oliviertassinari added a commit that referenced this pull request Dec 7, 2015
[Doc] Autogenerate the property description fo the AppBar
@oliviertassinari oliviertassinari merged commit 718c840 into mui:master Dec 7, 2015
@oliviertassinari oliviertassinari deleted the doc-autogenerated branch December 7, 2015 15:04
@alitaheri
Copy link
Member

Awesome 👍 🎉 🎉
Thanks for doing this @oliviertassinari

@shaurya947
Copy link
Contributor

👍 great initiative @oliviertassinari @subjectix

@frankf-cgn
Copy link

@oliviertassinari Really nice work and awesome idea to let the props-table beeing autogenerated!

@newoga
Copy link
Contributor

newoga commented Dec 8, 2015

Just noticed this, this is awesome! 👍

@zannager zannager added docs Improvements or additions to the documentation component: app bar This is the name of the generic UI component, not the React module! labels Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: app bar This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants