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

Block Editor: LinkControl: Resolve error when undefined value, "view" state #19856

Merged
merged 3 commits into from
Jan 28, 2020
Merged

Block Editor: LinkControl: Resolve error when undefined value, "view" state #19856

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
19b918e
Block Editor: LinkControl: Resolve error when undefined value, "view"…
aduth Jan 24, 2020
bc5e696
Block Editor: LinkControl: Document only url, title, opensInNewTab va…
aduth Jan 28, 2020
1e55842
Block Editor: LinkControl: Change documented example to reference kno…
aduth Jan 28, 2020
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
30 changes: 26 additions & 4 deletions packages/block-editor/src/components/link-control/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,28 @@
# Link Control

Renders a link control. A link control is a controlled input which maintains
a value associated with a link (HTML anchor element) and relevant settings
for how that link is expected to behave.

## Props

### value

- Type: `Object`
- Required: No

Current link value.

A link value contains is composed as a union of the default properties and any custom settings values.

Default properties include:

- id (`string|number`): Unique identifier of link.
- url (`string`): Link URL.
- title (`string`): Link title.
- type (`string`): Type of link entity.
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the possible values for type and subtype?

Copy link
Member Author

Choose a reason for hiding this comment

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

What are the possible values for type and subtype?

My understanding of the way it works today is that this inherits the same schema as the REST API search endpoint:

https://developer.wordpress.org/rest-api/reference/search-results/

(You can see there the possible values for this today)

I think a natural worry here could be that it ties too closely with the WordPress-specific idea of what a link can represent, but it seems to have been designed in a fairly generic sense, and I think this could extend to represent other "type"s of link entities.

#19827 is similar here in trying to group all manually-entered URLs under a "url" type.

Also some related discussion at #19387 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

It might help to document something here about how someone should expect to use these, but I'm not entirely clear what exactly. It should probably tie into the introduction of documentation surrounding the fetchLinkSuggestions context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a "type" for raw url?

Copy link
Member Author

Choose a reason for hiding this comment

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

As documented here, there would have to be something yes, even a subtype must be assigned.

Again, this is inherited largely from the schema for the search endpoint.

I might be inclined to think that it probably makes sense to allow them to be nullable or omitted, but we'd want to be quite explicit about that, and I think it would require one of:

  • Diverging from the search endpoint schema
  • Updating the search endpoint schema

Ties in as well with:

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing blocking here feel free to move forward but yeah the clearer the value format is, the easier it is to work with the component.

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'm glad you're thinking about it, and I do think it's an important decision to make, considering its impact on usability and consistency. I'm not sure how common it would be to manually create values for the component, but I don't like as in the second code snippet of #18061 (comment) that it would be considered invalid when providing a value to not also include the type, subtype, or even title of a link. I'd like to think it should be perfectly acceptable to pass { url: 'http://example.com' } as a value.

This might be something where a selected suggestion would be expected include these properties, but that it's not necessarily required for the value provided. It's a bit confusing, especially if we consider that most usage might look something like:

const [ value, setValue ] = useState();
return <LinkControl value={ value } onChange={ ( nextValue ) => setValue( nextValue ) } />;

With this code, we'd probably consider nextValue to be of the same general shape as the value passed.

It's also a question of: What of these details does LinkControl actually use when a value is assigned, and does it require them?

  • For type, id, and subtype, the component doesn't actually use them
  • For title, the component uses them, but does (or should) have fallbacks when it's missing or empty
  • url is the one which is both used and required

I wonder if the best way to move forward on this specific pull request would be to only document the title and url parts of the value, perhaps considering in a separate pull request to add a different "suggestion" type which can include the ID, type, and subtype.

title is an interesting one, because we probably want to allow it to be optional. But does that mean:

  • Allow it to be omitted altogether
  • Make it required, but allow it to be null
  • Make it required, and it must be a string (use an empty string '' if it's not assigned)

The first is probably the easiest from usability, but further reinforces this difference between a value type and a suggestion type.

Maybe, in the end, the best compromise would be one where the value type and suggestion type are technically distinct, but that value is aware of and would optionally accept any properties from a suggestion, even if it didn't strictly need or use them.

Example: https://www.typescriptlang.org/play/index.html#code/JYOwLgpgTgZghgYwgAgOoAUAyoDWBhAe3CgIBsA1OUgVxQG8BYAKGVeWABMB+ALmQGcwUUAHNkAH2QhqAWwBG0ADTM2yalFJ9BwkCOUs2YYGFIReAoaP2qwATwAOZrZd3W2-anLuPz2q8wBfZmZQSFhEFAxsEHwiITIAZWoREQhBYCJkCAAPSBAOfjQsXEJiMkoaehU2TmcdMUlpeSVq1iMTCDr-AzaHTot6t1YPLz6u10DgpgB6aeQAWkWl5ZWphCJBZAA3Klo+KJK4kgpdlABeZDo1DT4AcgALMDB7HlmcuBl7UwA6dZlb5ABADczHWIE2IByYAqeyK0ViZVISRSaSMmQujB67A4d0ez1e03enx+f1uQ2ummQDyeLze2Q+XwgvwI-3J7VMdzJrWQ3n6t3UpC5WJGvNxNKFwLWGzA21OqGM9wA8vY0SAqABRXJQOD7YoxUrxE6VZAY7m1Kl42mE+nEpmk8kCsX4ukMkksoU2YwcqkewxjKkC33DTyii3i-SSphgzagHakTgAOShMP6B31R0SyVS6XRlwpTqtRMZzP+gKBQA

I still think it's awkward to try to define id, subtype for a "raw URL" value 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to reply to your comment and noticed that I think we're on the same page for everything I think.

I wonder if the best way to move forward on this specific pull request would be to only document the title and url parts of the value, perhaps considering in a separate pull request to add a different "suggestion" type which can include the ID, type, and subtype.

While using the component, I didn't even know it contained type, suptype and id. so yeah title and url are more important.

The first is probably the easiest from usability, but further reinforces this difference between a value type and a suggestion type.

Is that bad? I think it's ok for different "types" of links. I also considered whether we should have a type: "raw" to clarify the difference but I guess the absence of "type" could mean the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The first is probably the easiest from usability, but further reinforces this difference between a value type and a suggestion type.

Is that bad? I think it's ok for different "types" of links. I also considered whether we should have a type: "raw" to clarify the difference but I guess the absence of "type" could mean the same thing.

Not necessarily, but I think it will be natural to observe that the two types share a fair few number of properties, and conclude that they are somehow related (or, the same).

- subtype (`string`): Subtype of link entity.

### settings

- Type: `Array`
Expand All @@ -33,14 +49,20 @@ An array of settings objects. Each object will used to render a `ToggleControl`
- Type: `Function`
- Required: No

Use this callback to take an action after a user set or updated a link.
The function callback will receive the selected item.
Value change handler, called with the updated value if the user selects a new link or updates settings.

```jsx
<LinkControl
onChange={ ( item ) => {
console.log( `The item selected has the ${ item.id } id.` );
onChange={ ( nextValue ) => {
console.log( `The item selected has the ${ nextValue.id } id.` );
}
/>
```

### showInitialSuggestions

- Type: `boolean`
- Required: No
- Default: `false`

Whether to present initial suggestions immediately.
100 changes: 79 additions & 21 deletions packages/block-editor/src/components/link-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,66 @@ import LinkControlSettingsDrawer from './settings-drawer';
import LinkControlSearchItem from './search-item';
import LinkControlSearchInput from './search-input';

/**
* Default properties associated with a link control value.
*
* @typedef WPLinkControlDefaultValue
*
* @property {string|number} id Unique identifier of link.
* @property {string} url Link URL.
* @property {string} title Link title.
* @property {string} type Type of link entity.
* @property {string} subtype Subtype of link entity.
* @property {boolean} [opensInNewTab] Whether link should open in a new
* browser tab. This value is only
* assigned if not providing a custom
* settings array.
*/

/**
* Custom settings values associated with a link.
*
* @typedef {{[setting:string]:any}} WPLinkControlSettingsValue
*/

/**
* Custom settings values associated with a link.
*
* @typedef WPLinkControlSetting
*
* @property {string} id Identifier to use as property for setting value.
* @property {string} title Human-readable label to show in user interface.
*/

/**
* Properties associated with a link control value, composed as a union of the
* default properties and any custom settings values.
*
* @typedef {WPLinkControlDefaultValue&WPLinkControlSettingsValue} WPLinkControlValue
*/

/** @typedef {(nextValue:WPLinkControlValue)=>void} WPLinkControlOnChangeProp */

/**
* @typedef WPLinkControlProps
*
* @property {(WPLinkControlSetting[])=} settings An array of settings objects. Each object will used to
* render a `ToggleControl` for that setting.
* @property {(search:string)=>Promise=} fetchSearchSuggestions Fetches suggestions for a given search term,
* returning a promise resolving once fetch is complete.
Comment on lines +71 to +72
Copy link
Member Author

Choose a reason for hiding this comment

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

This shouldn't have been included. It's not (any longer) a prop for the component.

* @property {WPLinkControlValue=} value Current link value.
* @property {WPLinkControlOnChangeProp=} onChange Value change handler, called with the updated value if
* the user selects a new link or updates settings.
* @property {boolean=} showInitialSuggestions Whether to present initial suggestions immediately.
*/

/**
* Renders a link control. A link control is a controlled input which maintains
* a value associated with a link (HTML anchor element) and relevant settings
* for how that link is expected to behave.
*
* @param {WPLinkControlProps} props Component props.
*/
function LinkControl( {
value,
settings,
Expand Down Expand Up @@ -157,7 +217,19 @@ function LinkControl( {

return (
<div className="block-editor-link-control">
{ ( ! isEditingLink ) && (
{ isEditingLink || ! value ?
<LinkControlSearchInput
value={ inputValue }
onChange={ onInputChange }
onSelect={ ( suggestion ) => {
setIsEditingLink( false );
onChange( { ...value, ...suggestion } );
} }
renderSuggestions={ renderSearchResults }
fetchSuggestions={ getSearchHandler }
onReset={ resetInput }
showInitialSuggestions={ showInitialSuggestions }
/> :
<Fragment>
<p className="screen-reader-text" id={ `current-link-label-${ instanceId }` }>
{ __( 'Currently selected' ) }:
Expand Down Expand Up @@ -191,27 +263,13 @@ function LinkControl( {
{ __( 'Edit' ) }
</Button>
</div>
<LinkControlSettingsDrawer
value={ value }
settings={ settings }
onChange={ onChange }
/>
</Fragment>
) }

{ isEditingLink && (
<LinkControlSearchInput
value={ inputValue }
onChange={ onInputChange }
onSelect={ ( suggestion ) => {
setIsEditingLink( false );
onChange( { ...value, ...suggestion } );
} }
renderSuggestions={ renderSearchResults }
fetchSuggestions={ getSearchHandler }
onReset={ resetInput }
showInitialSuggestions={ showInitialSuggestions }
/>
) }

{ ! isEditingLink && (
<LinkControlSettingsDrawer value={ value } settings={ settings } onChange={ onChange } />
) }
}
</div>
);
}
Expand Down