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

URLInput: Pass the the search result object to props.onChange #8662

Merged
merged 10 commits into from
Aug 13, 2018
181 changes: 181 additions & 0 deletions packages/editor/src/components/url-input/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
# `URLInputButton`

Render a URL input button that pops up an input to search for and select a post or enter any arbitrary URL.

## Properties

### `url: String`

*Required.* This should be set to the attribute (or component state) property used to store the URL.

### `onChange( url: String, post: Object ): Function`

*Required.* Called when the value changes. The second parameter defaults to an empty object unless the user selects a post from the suggestions dropdown. In those cases the `post` parameter will look like this:

```json
{
"id": 1,
"subtype": "page",
"title": "Sample Page",
"type": "post",
"url": "https://example.com/sample-page/",
"_links": {
"self": [ { "embeddable": true, "href": "https://example.com/wp-json/wp/v2/pages/1" } ],
"about": [ { "href": "https://example.com/wp-json/wp/v2/types/page" } ],
"collection": [ { "href": "https://example.com/wp-json/gutenberg/v1/search" } ]
}
}
```

## Example

{% codetabs %}
{% ES5 %}
```js
wp.blocks.registerBlockType( /* ... */, {
// ...

attributes: {
url: {
type: 'string'
},
text: {
type: 'string'
}
},

edit: function( props ) {
return wp.element.createElement( wp.editor.URLInputButton, {
className: props.className,
url: props.attributes.url,
onChange: function( url, post ) {
props.setAttributes( { url: url, text: post.title || 'Click here' } );
}
} );
},

save: function( props ) {
return wp.element.createElement( 'a', {
href: props.attributes.url,
}, props.attributes.text );
}
} );
```
{% ESNext %}
```js
const { registerBlockType } = wp.blocks;
const { URLInputButton } = wp.editor;

registerBlockType( /* ... */, {
// ...

attributes: {
url: {
type: 'string',
},
text: {
type: 'string',
},
},

edit( { className, attributes, setAttributes } ) {
return (
<URLInputButton
url={ attributes.url }
onChange={ ( url, post ) => setAttributes( { url, text: post.title || 'Click here' } ) }
/>
);
},

save( { attributes } ) {
return <a href={ attributes.url }>{ attributes.text }</a>;
}
} );
```
{% end %}

# `URLInput`

Renders the URL input normally wrapped by `URLInputButton`.
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean? Oh, wait, I think I see. I think this should be something like:

Renders a URL input field; this component should typically be wrapped in a URLInputButton component.

right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think saying this might be an improvement:

Renders the URL input field used by the URLInputButton component. It can be used directly to display the input field in different ways such as in a Popover or inline.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, that's great 👍


## Properties

### `value: String`

*Required.* This should be set to the attribute (or component state) property used to store the URL.

### `onChange( url: String, post: Object ): Function`

*Required.* Called when the value changes. This is the same as the `onChange` prop described above for `URLInputButton`.
Copy link
Member

Choose a reason for hiding this comment

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

Saying "this is the same" is a bit confusing to me–does that mean it's functionally the same or it's literally passed from URLInputButton to this component? If it's just that it behaves similarly I think it's worth duplicating the documentation because it's annoying to have to hunt around a file for docs 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's literally passed directly from the URLInputButton component. But I think there's no harm in duplicating it for the sake of ease as you say 👍


### `autoFocus: Boolean`

*Optional.* By default, the input will gain focus when it is rendered as typically it is used in combination with a `Popover` in `URLInputBUtton`. If you are rendering the component all the time set this to `false`.
Copy link
Member

Choose a reason for hiding this comment

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

A comma after "rendered" would improve readability here. I also think you could tweak the second sentence:

By default, the input will gain focus when it is rendered, as typically it is used in combination with a Popover in URLInputBUtton. If you are not conditionally rendering this component: set this to false.


## Example

{% codetabs %}
{% ES5 %}
```js
wp.blocks.registerBlockType( /* ... */, {
// ...

attributes: {
url: {
type: 'string'
},
text: {
type: 'string'
}
},

edit: function( props ) {
return wp.element.createElement( wp.editor.URLInput, {
className: props.className,
value: props.attributes.url,
onChange: function( url, post ) {
props.setAttributes( { url: url, text: post.title || 'Click here' } );
}
} );
},

save: function( props ) {
return wp.element.createElement( 'a', {
href: props.attributes.url,
}, props.attributes.text );
}
} );
```
{% ESNext %}
```js
const { registerBlockType } = wp.blocks;
const { URLInput } = wp.editor;

registerBlockType( /* ... */, {
// ...

attributes: {
url: {
type: 'string',
},
text: {
type: 'string',
},
},

edit( { className, attributes, setAttributes } ) {
return (
<URLInput
className={ className }
value={ attributes.url }
onChange={ ( url, post ) => setAttributes( { url, text: post.title || 'Click here' } ) }
/>
);
},

save( { attributes } ) {
return <a href={ attributes.url }>{ attributes.text }</a>;
}
} );
```
{% end %}
12 changes: 6 additions & 6 deletions packages/editor/src/components/url-input/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,9 @@ class URLInput extends Component {
this.suggestionsRequest = request;
}

onChange( event ) {
onChange( event, post = {} ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this post argument is useless because it's always undefined anyway (default value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not totally sure I follow your meaning, do you mean leave out the default value or change the call to this.props.onChange() to this.props.onChange( ...arguments ) instead? I felt this was a bit more readable and the documentation would be simpler if the type was always consistent.

const inputValue = event.target.value;
this.props.onChange( inputValue );
this.props.onChange( inputValue, post );
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make the post argument to the onChange prop nullable and just avoid passing it here? If I understand properly this is a special case where don't select anything from the suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only place this.props.onChange() is called, not passing it here would remove the intended change wouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh my bad, thought selectLink() called this.onChange() 🤦‍♂️

this.updateSuggestions( inputValue );
}

Expand Down Expand Up @@ -168,14 +168,14 @@ class URLInput extends Component {
if ( this.state.selectedSuggestion !== null ) {
event.stopPropagation();
const post = this.state.posts[ this.state.selectedSuggestion ];
this.selectLink( post.url );
this.selectLink( post.url, post );
}
}
}
}

selectLink( link ) {
this.props.onChange( link );
selectLink( link, post ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the first argument from this method, as it's already in post.url

this.props.onChange( link, post );
this.setState( {
selectedSuggestion: null,
showSuggestions: false,
Expand Down Expand Up @@ -227,7 +227,7 @@ class URLInput extends Component {
className={ classnames( 'editor-url-input__suggestion', {
'is-selected': index === selectedSuggestion,
} ) }
onClick={ () => this.selectLink( post.url ) }
onClick={ () => this.selectLink( post.url, post ) }
aria-selected={ index === selectedSuggestion }
>
{ decodeEntities( post.title ) || __( '(no title)' ) }
Expand Down