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.

Sign up for GitHub

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

Jump to bottom

Fix/update docs alignment matrix control #34624

Merged
merged 2 commits into from
Sep 8, 2021
Merged

Fix/update docs alignment matrix control #34624

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e7b778
Update example to import correctly and use a more verbose example in …
Sep 7, 2021
bd9e7bd
Add prop details.
Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 50 additions & 3 deletions packages/components/src/alignment-matrix-control/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,61 @@ AlignmentMatrixControl components enable adjustments to horizontal and vertical
## Usage

```jsx
import { AlignmentMatrixControl } from '@wordpress/components';
import { __experimentalAlignmentMatrixControl as AlignmentMatrixControl } from '@wordpress/components';
import { useState } from '@wordpress/element';

const Example = () => {
const [ alignment, setAlignment ] = useState( 'center center' );
const [alignment, setAlignment] = useState('center center');

return (
<AlignmentMatrixControl value={ alignment } onChange={ setAlignment } />
<AlignmentMatrixControl
value={alignment}
onChange={(newAlignment) => setAlignment(newAlignment)}
/>
);
};
Comment on lines +8 to 20
Copy link
Contributor

Choose a reason for hiding this comment

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

Very very minor, but — it'd be great if the example snippet followed the repo's prettier formatting rules

Suggested change
import { __experimentalAlignmentMatrixControl as AlignmentMatrixControl } from '@wordpress/components';
import { useState } from '@wordpress/element';
const Example = () => {
const [ alignment, setAlignment ] = useState( 'center center' );
const [alignment, setAlignment] = useState('center center');
return (
<AlignmentMatrixControl value={ alignment } onChange={ setAlignment } />
<AlignmentMatrixControl
value={alignment}
onChange={(newAlignment) => setAlignment(newAlignment)}
/>
);
};
import { __experimentalAlignmentMatrixControl as AlignmentMatrixControl } from '@wordpress/components';
import { useState } from '@wordpress/element';
const Example = () => {
const [ alignment, setAlignment ] = useState( 'center center' );
return (
<AlignmentMatrixControl
value={ alignment }
onChange={ ( newAlignment ) => setAlignment( newAlignment ) }
/>
);
};

```

## Props

The component accepts the following props:
### className

The class that will be added with "component-alignment-matrix-control" to the classes of the wrapper <Composite/> component.
If no className is passed only "component-alignment-matrix-control" is used.
Comment on lines +28 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that some older READMEs have a similar paragraph, but from now on let's not mention the "component-alignment-matrix-control" className in public docs


- Type: `String`
- Required: No

### id

Unique ID for the component.
- Type: `String`
- Required: No
### label
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nitpick: most headers used for props are missing an empty line above them

Suggested change
### label
### label


If provided, sets the aria-label attribute of the wrapper <Composite/> component.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
If provided, sets the aria-label attribute of the wrapper <Composite/> component.
Accessible label. If provided, sets the `aria-label` attribute of the underlying <Composite/> component.


- Type: `String`
- Required: No
- Default: `Alignment Matrix Control`
### defaultValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add the value prop to the docs? It looks like it's currently missing


If provided, sets the default alignment value.
- Type: `String`
- Required: No
- Default: `center center`

### onChange

A function that receives the updated alignment value.

- Type: `function`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we replace types to be closer to how TypeScript types are described?

In this case, it could be something like ( nextValue: string ) => void

Other changes would be:

  • from String to string
  • from Number to number

- Required: No
- Default: `noop`
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that the default value for this prop is a noop function, but I'd remove this line from the README, as it may confuse a reader instead of adding value

### width

If provided, sets the width of the wrapper <Composite/> component.
- Type: `Number`
- Required: No
- Default: `92`