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

roborourke · 2018-08-07T14:29:19Z

Description

The URLInput component is great however the number of use cases it can support increases a huge amount if it makes the search result object available to the onChange() prop as well.

Example use cases:

Component can be used as a simple post picker
Retrieving the post title as well to update block attributes eg. link text
Fetching other / additional post data using the _links property of the search result object

How has this been tested?

I made a copy of the component with the proposed modifications locally and used it as a post picker. I updated not only the URL but was able to set other attributes such as a post title and fetch the post's featured image.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

This component is great however the number of use cases it can support increases a huge amount if it makes the search result object available to the `onChange()` prop as well. Example use cases: - Retrieving the post title, ID and post type, as well as URL to update block attributes - Fetching other / additional post data using the `_links` property of the search result object

youknowriad

LGTM overall, I left some small comments and I'd appreciate other opinions on whether this is a good idea or not. cc @WordPress/gutenberg-core

youknowriad · 2018-08-08T11:18:08Z

packages/editor/src/components/url-input/index.js

@@ -131,9 +131,9 @@ class URLInput extends Component {
 		this.suggestionsRequest = request;
 	}

-	onChange( event ) {
+	onChange( event, post = {} ) {


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

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.

youknowriad · 2018-08-08T11:19:07Z

packages/editor/src/components/url-input/index.js

 		const inputValue = event.target.value;
-		this.props.onChange( inputValue );
+		this.props.onChange( inputValue, post );


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.

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

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

@youknowriad

Thanks for the review @youknowriad

youknowriad · 2018-08-08T12:05:05Z

packages/editor/src/components/url-input/index.js

 				}
 			}
 		}
 	}

-	selectLink( link ) {
-		this.props.onChange( link );
+	selectLink( link, post ) {


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

youknowriad

LGTM 👍 Anyone for another opinion here?

tofumatt

I think this is a solid change but there are documentation tweaks I think are worth doing before merging. I can make them in a few hours if no one can get to them beforehand–if they're made before I get the chance I'll re-review and merge when I'm back 😄

tofumatt · 2018-08-08T19:00:43Z

packages/editor/src/components/url-input/README.md

+
+# `URLInput`
+
+Renders the URL input normally wrapped by `URLInputButton`.


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?

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.

Agreed, that's great 👍

tofumatt · 2018-08-08T19:02:30Z

packages/editor/src/components/url-input/README.md

+
+### `onChange( url: String, ?post: Object ): Function`
+
+*Required.* Called when the value changes. This is the same as the `onChange` prop described above for `URLInputButton`.


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 😄

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 👍

tofumatt · 2018-08-08T19:03:59Z

packages/editor/src/components/url-input/README.md

+
+### `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`.


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.

@tofumatt

Add some changes based on feedback from @tofumatt, much appreciated!

tofumatt · 2018-08-10T21:29:21Z

Changes are good; thanks a bunch!

roborourke · 2018-08-14T10:59:33Z

@tofumatt @youknowriad thank you!

roborourke added 6 commits August 7, 2018 15:21

Add a component readme

3895b87

Fix json syntax

e8983cf

Fix code example indentation

4ea3d51

document URLInputButton as well

add432d

No autoFocus prop for button

af19bed

youknowriad reviewed Aug 8, 2018

View reviewed changes

gziolo added the [Type] Enhancement A suggestion for improvement. label Aug 8, 2018

roborourke added 2 commits August 8, 2018 12:58

Remove unneeded arg on onChange method

00378a3

Thanks for the review @youknowriad

Update docs to reflect nullable post arg [ci skip]

7d9b96e

youknowriad reviewed Aug 8, 2018

View reviewed changes

remove first arg from selectLink

e5f7313

youknowriad approved these changes Aug 8, 2018

View reviewed changes

tofumatt requested changes Aug 8, 2018

View reviewed changes

Improve readability

734522c

Add some changes based on feedback from @tofumatt, much appreciated!

tofumatt approved these changes Aug 10, 2018

View reviewed changes

youknowriad merged commit fd6aa36 into WordPress:master Aug 13, 2018

roborourke deleted the patch-1 branch August 14, 2018 10:59

snyk-bot mentioned this pull request Nov 18, 2020

[Snyk] Fix for 1 vulnerabilities wpscholar/gutenberg#12

Open

snyk-bot mentioned this pull request Dec 17, 2020

[Snyk] Security upgrade jest from 25.5.4 to 26.0.0 wpscholar/gutenberg#14

Open

This was referenced Mar 30, 2021

[Snyk] Security upgrade @jest/reporters from 25.5.1 to 26.0.0 wpscholar/gutenberg#23

Open

[Snyk] Fix for 1 vulnerabilities wpscholar/gutenberg#24

Open

This was referenced Sep 15, 2021

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#20

Open

[Snyk] Security upgrade @jest/reporters from 25.5.1 to 26.0.0 Verdinjoshua26/gutenberg#22

Open

This was referenced Jun 23, 2023

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#77

Open

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#79

Open

Verdinjoshua26 mentioned this pull request Sep 7, 2023

[Snyk] Security upgrade jest from 25.5.4 to 26.0.0 Verdinjoshua26/gutenberg#85

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

roborourke commented Aug 7, 2018 •

edited

Loading

youknowriad left a comment

youknowriad Aug 8, 2018

roborourke Aug 8, 2018

youknowriad Aug 8, 2018

roborourke Aug 8, 2018

roborourke Aug 8, 2018

youknowriad Aug 8, 2018

youknowriad left a comment

tofumatt left a comment

tofumatt Aug 8, 2018

roborourke Aug 9, 2018

tofumatt Aug 10, 2018

tofumatt Aug 8, 2018

roborourke Aug 9, 2018

tofumatt Aug 8, 2018

tofumatt commented Aug 10, 2018

roborourke commented Aug 14, 2018


		# `URLInput`

		Renders the URL input normally wrapped by `URLInputButton`.


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

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


		### `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`.

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

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

Conversation

roborourke commented Aug 7, 2018 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented Aug 10, 2018

roborourke commented Aug 14, 2018

roborourke commented Aug 7, 2018 •

edited

Loading