Add components examples in README.md files - Part 1 #8267
Conversation
|
|
||
| if ( isDisabled ) { | ||
| form = <Disabled>{ form }</Disabled>; | ||
| class ToggleDisable extends React.Component { |
There was a problem hiding this comment.
We favor functional components, it should stay as it was. Using Component class with lifecycle methods is more complex, requires more code and might be subject of change in the future. Actually, React 16.3 is a great example how those API can get deprecated.
| ## Usage | ||
|
|
||
| ```jsx | ||
| class MyClipboardButton extends React.Component { |
There was a problem hiding this comment.
This example can also be expressed as functional component wrapped with withState higher-order component (HOC). I believe it is code this way in the existing codebase.
| @@ -8,17 +8,19 @@ BaseControl component is used to generate labels and help text for components ha | |||
|
|
|||
| Render a BaseControl for a textarea input: | |||
| ```jsx | |||
| <BaseControl | |||
There was a problem hiding this comment.
Can we also include import statement to make it possible to extract code as is and run it without any modifications?
There was a problem hiding this comment.
Yeah, that would be great but I'm afraid I have a limitation here.
I'm using react-live for compiling and running these snippets of code in real time, so we avoid to change our build process.
That forces us to import the dependencies outside the snippets in order to define the scope needed by react-live to run the extracted code.
There was a problem hiding this comment.
Anyway, I think we should maintain them in the examples in order to provide documentation as complete as possible. We can always ignore them when running the snippets (you can check how I'm handling that here: https://github.com/Automattic/wp-calypso/pull/26367/files#diff-d0c3bde7d6504cb5de69c0488b877342R36)
mmtr
changed the title
|
@gziolo as discussed, I've split this PR into 2 parts, so this first one should be ready to review :) thanks! |
|
note that some of the current README files already provide working examples, so I have not modified them |
There was a problem hiding this comment.
Great work on this PR. I really appreciate your effort to improve those examples. In particular, the fact that you converted all of them to be functional ❤️
I left my comments. They shouldn't be difficult to address. If you have any concerns or disagree with anything, let me know. Happy to discuss.
| ), | ||
| import { Autocomplete } from '@wordpress/components'; | ||
|
|
||
| function FreshFruitAutocomplete() { |
There was a problem hiding this comment.
Out of curiosity, don't you need to export those components to be able to use them in Calypso devdocs? For the purpose of docs it's fine as is. However, if that would help, don't hesitate to add export statements.
There was a problem hiding this comment.
react-live doesn't need it. As long as you inject a string with either a component class, a functional component or JSX code, it will be able to render it: https://react-live.kitten.sh/
There was a problem hiding this comment.
How do you feel about using the version which calls render?
const Wrapper = ({ children }) => (
<div style={{
background: 'papayawhip',
width: '100%',
padding: '2rem'
}}>
{children}
</div>
)
const Title = () => (
<h3 style={{ color: 'palevioletred' }}>
Hello World!
</h3>
)
render(
<Wrapper>
<Title />
</Wrapper>
)3rd example on the demo page.
There was a problem hiding this comment.
It's not trivial.
That render call should be performed in Calypso (it doesn't make sense to include it in the Gutenberg examples), but how Calypso can know what are the components that need to be rendered?
For rendering the Autocomplete example it should use render( <FreshFruitAutocomplete /> ), but for the Button example it should use render( <MyButton /> ). But there is no way for getting this in a dynamic way.
Maybe we can use always the My<ComponentName> pattern and assume that in Calypso. But we need to be sure that the name of the variables doesn't change in the example or we'll broke the Calypso's DevDocs. What do you think?
There was a problem hiding this comment.
I missed that comment. It seems like a good pattern, let's discuss further in the 2nd part of the PR where those changes are applied.
| @@ -1,5 +1,5 @@ | |||
| BaseControl | |||
| ======= | |||
| =========== | |||
There was a problem hiding this comment.
To avoid some hassle with adding those equality signs, you can also simply use:
# BaseControl
We use this form in README.md files in the root folder of each npm package.
| return ( | ||
| <Button | ||
| isPrimary | ||
| href="https://wordpress.com" |
There was a problem hiding this comment.
Gutenberg, it's a community project. Let's use https://wordpress.org/ instead 😄
| heading="User" | ||
| label="Is author" | ||
| help="Is the user a author or not?" | ||
| onChange={ () => {} } |
There was a problem hiding this comment.
I think it would make sense to include checked property, too. Otherwise this example won't be very useful. In such case onChange would need to be update. We can use something similar to what we have in ClipboardButton using withState.
| } )( ( { hasCopied, setState } ) => ( | ||
| <ClipboardButton | ||
| isPrimary | ||
| text="WordPress" |
There was a problem hiding this comment.
It might to make it easier to understand if we would add some description inside text prop, .e.g.: Text to be copied..
| } | ||
| withState( { | ||
| tokens: [], | ||
| suggestions: ['Africa', 'America', 'Antarctica', 'Asia', 'Europe', 'Oceania'], |
There was a problem hiding this comment.
Missing spaces after [ and before ], sorry about WordPress conventions 😄
| import { KeyboardShortcuts } from '@wordpress/components'; | ||
| import { withState } from '@wordpress/compose'; | ||
|
|
||
| withState( { |
| function MyMenuGroup() { | ||
| return ( | ||
| <MenuGroup label="Settings"> | ||
| <div>Setting 1</div> |
There was a problem hiding this comment.
It might be better to put MenuItem in here:
<MenuGroup label="Settings">
<MenuItem>Setting 1</MenuItem>
<MenuItem>Setting 2</MenuItem>
</MenuGroup>This is how it should be used in general.
| import { MenuItem } from '@wordpress/components'; | ||
| import { withState } from '@wordpress/compose'; | ||
|
|
||
| withState( { |
| import { MenuGroup, MenuItemsChoice } from '@wordpress/components'; | ||
| import { withState } from '@wordpress/compose'; | ||
|
|
||
| withState( { |
gziolo
added
[Package] Components
|
It looks solid, I don't want to block you from progressing on your work because of nitpicks. If you find a way to include Awesome work and very quick iterations loop 💯 |
|
There are only |
The aim of this PR is to include working examples in the components documentation.
Based on this idea from @gziolo for avoiding to have Gutenberg examples in Calypso.
This task is continued on #8338
Components (Part 1)