Declare and override autocompleters via filter #4609

brandonpayton · 2018-01-20T00:49:53Z

Description

This is a PR for declaring and overriding autocompleters during block registration.

How Has This Been Tested?

I have manually tested with an ad hoc plugin, have added unit tests, and will be adding more.

Types of changes

Autocompleters have a type property so they can be identified.
Separate option retrieval from rendering option labels and keywords, enabling us to override retrieval, labeling, and keywords separately.
Make completers more declarative by removing their onSelect function and replacing it with getOptionCompletion that returns an action/value pair. Currently supported actions are insert-at-caret and replace-block. This detangles completers from runtime dependencies on things such as the onReplace function. Now, the Autocompleter component uses the completion action to decide how to handle the result.
Ignore the concept of option value and simply pass the selected item to a completer's getOptionCompletion function. This isn't a stance against the concept of value, but it seemed like unnecessary indirection in the current implementation.
Export a set of default autocompleters from blocks/autocompleters.
Update RichText to include autocompletion support, including a new autocompleters prop that can be used to specify completers in place of the default.
Add a blocks/hooks/default-autocompleters module to provide a default list of completers for blocks that don't explicitly provide their own.
The blocks.Autocomplete.completers filter is applied when the Autocomplete component is first focused after receiving a new list of completers.

Checklist:

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

brandonpayton · 2018-01-24T18:12:37Z

If others agree this approach is reasonable, I'd also like to add a filter for transforming the default list of completers. For my use case, I'd want to add a completer for cross-posting to different P2s (e.g., +otherp2).

gziolo · 2018-01-31T12:57:17Z

components/autocomplete/index.js

+
+		// Filter the options getter itself because we only want to invoke one,
+		// whether it be default or custom.
+		const optionsGetter = applyFilters( `autocomplete.${ type }.options.getter`, completer.getOptions );


We need to standardize the way we name filters to have one pattern that is easy to follow. So far we were prefixing the hooks name with the package name followed by the component's name. So, in this case it would be something like: components.Autocomplete. Another thing here is that we can combine those 3 filters into one as follows and call it once only when the component gets initialized:

this.props.completers = applyFilters( `components.Autocomplete.completers', this.props.defaultCompleters );

gziolo · 2018-01-31T13:05:05Z

@brandonpayton, you can totally go for it. It's a valid use case and we should support a wide range of extensibility options. My only concern that your initial proposal it too granular and it would make it harder to discover for plugin developers. Another concern I have is that ideally all hooks should be applied only once when a component gets mounted. If we want to allow to dynamically add/remove/update hooks we can do it using event listeners provided by the hooks package. See the example Higher-Order component which is meant to hide implementation details of this behavior in a completely different context: https://github.com/WordPress/gutenberg/blob/master/components/higher-order/with-filters/index.js#L24. I would be more than happy to help to implement a similar wrapper component that would be tailored to solve the issue we have here. Let me know if you have more questions. I'm happy to discuss all the ideas I shared.

brandonpayton · 2018-02-01T06:56:59Z

Thanks for the review, encouragement, and guidance, @gziolo! I think you're right about the granularity, especially for a first pass. I'll update with a single filter that runs after the component is mounted.

gziolo · 2018-02-01T08:30:35Z

I noticed that @noisysocks is working on some refactoring related to autocompleters in #4769. Cross-linking for better visibility :)

brandonpayton · 2018-02-03T00:12:51Z

@noisysocks and I spoke briefly yesterday. Autocompleting reusable blocks means the completer needs access to the latest list.

I spent some time thinking today, and here's what I'm seeing.

The getOptions method should simply return a promise for raw options data.
Rendering, keywords, and value should be provided by separate functions that take an option argument. They are different concerns, and this will allow each to be overridden separately.
Add a components.Autocomplete.completers filter.
Consider adding a filter for each completer type after the above filter is applied (e.g., components.Autocomplete.completers.blocks).
The default blocks completer should provide a working getOptions that hits the REST API.
Via a filter mentioned above, the editor can override the blocks completer getOptions with one that uses the editor store.

Here's an example of what I'm imagining for the blocks completer.

{
	completes: 'blocks',
	className: 'blocks-autocompleters__block',
	triggerPrefix: '/',
	getOptions,
	getOptionKeywords,
	renderOption,
	allowContext,
	getOptionValue,
	onSelect,
}

I'm interested in your thoughts. If you have time, let's talk more on Monday.

brandonpayton · 2018-02-05T05:17:14Z

The default blocks completer should provide a working getOptions that hits the REST API.

Via a filter mentioned above, the editor can override the blocks completer getOptions with one that uses the editor store.

I may be wrong about this if the only expected context for a blocks completer is the editor. I've been assuming the editor only applies when editing a post, but if it also will be used in things like customization, then it probably doesn't make sense to provide a default getOptions that doesn't use the editor store.

I think the main thing for this PR is that option retrieval should be separated from rendering, so each can be overridden separately.

gziolo · 2018-02-06T09:37:21Z

@brandonpayton, I'm looking forward to seeing this PR updated. I need to go through the code to better understand how this would work. I don't know that well the internal implementation of autocompleters :)

brandonpayton · 2018-02-06T22:50:44Z

@gziolo I'm eager to work on this today but have to take care of something first. It might have to wait until tomorrow. Thanks for your comments and interest!

brandonpayton · 2018-02-07T20:14:14Z

I updated the PR to...

Separate option retrieval from rendering.
Fix existing tests and add a couple more.
Remove the autocomplete-related filters because I want to explore overriding autocompletion at the block registration level first.

I am working on...

Declare autocompleters with block registration.
Move to a single Autocomplete instance at the top level, likely wrapping WritingFlow.
- We can look up the current block and its autocompleters based on the input event target.
- This removes the question of where and when to wrap with Autocomplete.

brandonpayton · 2018-02-09T07:18:43Z

I ran into serious a11y questions with moving to a single top-level Autocomplete instance and am keeping multiple instances for now.

I updated Autocomplete to look up completers based on its containing block type but still need to update the tests. I also changed completers to simply return a result for a selected option and made it the responsibility of Autocomplete to insert text or replace the current block. This is the general idea, but it probably needs some cleanup.

I'd still like a way for autocompletion to automatically apply for blocks with defined autocompleters, but I'm not sure how to do that in an accessible way since autocomplete-related aria attributes and UI should be directly associated with each input field.

brandonpayton · 2018-02-10T04:11:42Z

Currently, <Autocomplete> looks up autocompleters by finding the containing block's DOM node, getting it's data-type, and looking up the block type settings. This is too entangled.

I'm thinking autocompleters should be added to the context with BlockEdit if they exist for a given block type. Then <Autocomplete> can use that context.

brandonpayton · 2018-02-12T15:07:39Z

I'm thinking autocompleters should be added to the context with BlockEdit if they exist for a given block type. Then <Autocomplete> can use that context.

I'm thinking Autocomplete should remain a simple props-driven @wordpress/components component and that we should create something like a ContextualAutocomplete component to live under @wordpress/blocks and receive autocompleters from context added by BlockEdit (which will know the autocompleters registered for a block).

This will allow autocompleters to be declared along with block registration and overridden at that granularity. To support blocks with multiple text inputs and different autocompleters for each, this will be insufficient. I'm thinking we should get this working at the block level first and iterate if we need further granularity.

gziolo · 2018-02-12T15:48:12Z

blocks/autocompleters/index.js

+export function blockAutocompleter() {
+	let promisedOptions;
+
+	const getOptions = () => promisedOptions || ( promisedOptions = Promise.resolve(


If you want to make sure this is called only once you can use lodash.once

const getOptions = once( () => Promise.resolve... );

Thanks for the tip! That's cleaner.
I'll make the update.

gziolo · 2018-02-12T15:49:33Z

blocks/autocompleters/index.js

-	const onSelect = ( blockName ) => {
-		onReplace( createBlock( blockName ) );
-	};
+	const getOptionResult = blockData => createBlock( blockData.name );


I don't know this code so just wanted to double check why to remove onReplace here.

It started with a dependency issue when moving completers to block registration. We didn't yet have an onReplace.

Then I thought completers should be more declarative anyway. The presence of an onSomething name made me think about that. Why should a completer know that it is replacing something in the editor? The idea is that the editor has a better idea of what it wants to do with the result of a completer selection.

I think it ensures we remove the existing paragraph block.

The Autocomplete component now calls onReplace instead when a block is returned.
ae62091#diff-8d7b3f0b32214bb1340578e3be07ed96R176

The idea is that a completer should not be involved in making actual changes to the content.

gziolo · 2018-02-12T15:56:26Z

blocks/autocompleters/index.js

+		<span key="name" className="blocks-autocompleters__user-name">{ user.name }</span>,
+		<span key="slug" className="blocks-autocompleters__user-slug">{ user.slug }</span>,
+	];
+


I like this part, it makes it nicely composable out of smaller pieces.

gziolo · 2018-02-12T16:12:17Z

components/autocomplete/index.js

+		if ( getOptionResult ) {
+			const replacement = getOptionResult( option.value, range, query );
+
+			if ( replacement && typeof replacement === 'object' && getBlockType( replacement.name ) ) {


Is there maybe an existing API method which validated if the input is a block? There is a similar logic in here:
https://github.com/WordPress/gutenberg/blob/master/blocks/block-edit/index.js#L22

We might want to extract this code and put in the method that would be used it in both places.

Agreed. I'll look at that.

I took a look and am wondering whether it is worth it to a function for the check. The only thing in common is that both make sure a truthy value is returned from getBlockType. Defining an isBlockType function in blocks/api/registration may allow us to be more direct though.

Currently, I simply moved the conditions from components/Autocomplete to blocks/contextual-autocomplete in the form of the isReplacement prop.

gziolo · 2018-02-12T16:21:47Z

blocks/autocompleters/test/index.js

-			] );
+		const completer = blockAutocompleter( { } );
+		const renderedLabels = getBlockTypes().map(
+			blockType => completer.renderOptionLabel( blockType )


It looks like you can perform all assertions inside this loop.

gziolo · 2018-02-12T16:26:35Z

blocks/autocompleters/test/index.js

+		renderedLabels.forEach( label => {
+			// Only verify that a populated label is returned.
+			// It is likely to be fragile to assert that the contents are renderable by @wordpress/element.
+			if ( Array.isArray( label ) ) {


Or can we make it more explicit what is what? At the moment it might work, but it's hard to decipher what we are checking here.

gziolo · 2018-02-12T16:29:38Z

blocks/library/paragraph/index.js

-					blockAutocompleter( { onReplace } ),
-					userAutocompleter(),
-				] }>
+				<Autocomplete getBlockType={ getBlockType } onReplace={ onReplace }>


We might want to iterate on getBlockType passed as prop. Maybe it should be a part of the autocompleter object. Not sure.

This will go away when I make the update for BlockEdit to provide autocompleters via context. There will be no need to find the containing block type to provide autocompletion context because that will be provided naturally via React context.

gziolo · 2018-02-12T16:30:13Z

blocks/library/paragraph/index.js

@@ -254,6 +251,11 @@ export const settings = {
 		],
 	},

+	autocompleters: [


It probably makes the most sense to have it here 👍 Let's see how it goes :)

Is there a particular use case we have in mind for having a block type define what autocompleters it wants? (I like to err on the side of you aren't gonna need it, especially since registerBlockType is a public API.)

You might want different autocompleters per block. A concrete example is that a block autocompleter makes sense in a paragraph but not in the caption field of a cover image. If you offer block completion there, it is more likely an opportunity for surprising replacement than a feature IMO.

What If you want different autocompleters per RichText? is this a valid use case? could this be a prop of RichText instead?

I'm not sure, but it seems reasonable to want different autocompleters per RichText. I'm not sure what overriding should look like in that case though.

I'm wondering whether this PR is trying to do too much and, in retrospect, would like to have created an autocompletion extensibility issue for discussion and smaller PRs. But if we can adjust in the future, maybe it would be good to continue as-is and get more feedback once merged.

Sounds good to me @brandonpayton

gziolo · 2018-02-12T16:32:08Z

components/autocomplete/index.js

+			const blockNode = inputNode.closest( '.editor-block-list__block' );
+			const blockName = get( blockNode, 'dataset.type', '' );
+			completers = blockName ?
+				get( getBlockType( blockName ), 'autocompleters', [] ) :


Yes, we might want to inject this, too. This is really interesting :)

brandonpayton · 2018-03-21T20:36:47Z

I updated this PR according to review notes made by @pento and believe it is ready for another review.

Rename type property to name
Default to inserting completion. Don’t require action/value pair from getOptionCompletion.
Allow filters to mutate completers
Rename renderOptionLabel to getOptionLabel for consistency.
Rename getOptions to options and allow it to be an array, a function that returns an array, or a function that promises an array. This makes the API easier to use while we internally continue to use a single async code path for consuming completer options.

cc @gziolo

pento · 2018-03-21T23:23:11Z

The API for creating autocompleters is feeling a lot more accessible, nice work @brandonpayton!

Could you add a README.md to the blocks/autocompleters folder, outlining what each of the properties are? There are a few concepts that plugin developers will need to wrap their heads around here, having a reference for them to look at would be helpful.

Other than that, 👍🏻 from me for the API. @gziolo, could you review the internals?

gziolo

I started the review. I left a few comments. I will download all code tomorrow and perform some testing.

gziolo · 2018-03-22T14:49:16Z

blocks/library/button/test/__snapshots__/index.js.snap

-      contenteditable="true"
-      data-is-placeholder-visible="true"
-    />
+    <div>


There are 3 additional divs in here, at least 2 of them look obsolete. It should be investigated further. Hopefully, it can be fixed.

I noticed this as well and was initially puzzled, but they are divs added by the Autocomplete component now used within RichText:

Autocomplete adds a div to wrap TinyMCE, adding event listeners and a sibling popup.

withFocusOutside wraps Autocomplete and adds a parent div because it adds event listeners.

The autocompleters hook adds another parent div in order to add its own event listeners.

I dislike this but do not currently see a better solution. I would love to be able to add event listeners to a Fragment (mentioned here as a possible future capability) to avoid rendering the elements. I'll give it some thought. Do you have any suggestions?

gziolo · 2018-03-22T15:06:07Z

docs/extensibility.md

+{% ES5 %}
+```js
+// Our completer
+function abbreviationCompleter( completers ) {


completers is not used inside the function, should be removed.

gziolo · 2018-03-22T15:06:17Z

docs/extensibility.md

+{% ESNext %}
+```js
+// Our completer
+function abbreviationCompleter( completers ) {


completers is not used inside the function, should be removed.

gziolo · 2018-03-22T15:10:38Z

blocks/hooks/autocompleters.js

+			}
+		}
+
+		updateFilteredCompleters( nextCompleters ) {


It can be also simplified to:

updateFilteredCompleters( nextCompleters = defaultCompleters ) {

by using a default param value.

Thanks, I'm not sure how I missed that :). Based on one of your other comments, I've updated the PR to provide defaults via a filter instead, so this no longer applies.

gziolo · 2018-03-22T15:12:25Z

blocks/hooks/autocompleters.js

+
+			this.setState( {
+				hasStaleCompleters: false,
+				completers: applyFilters(


There is also hasFilter method which would be useful here to avoid state updates when there are no filters registered.

In this case, the state must always be updated after new completer props are received because the filtered completers are maintained in the state. This is done on demand when the component first contains focus after receiving new completer props.

We can still use hasFilter to avoid copying the completers list when there is no filter added, and I'm making that change now.

gziolo · 2018-03-22T15:15:55Z

blocks/hooks/autocompleters.js

+ * @param  {Function} Autocomplete Original component.
+ * @return {Function}              Wrapped component
+ */
+export function filterAutocomplete( Autocomplete ) {


There is a convention to prefix all HOCs with with prefix. In this case, it might be better to name it withFilteredAutocompleters.

gziolo · 2018-03-22T15:20:12Z

blocks/hooks/test/autocompleters.js

+			wrapper.find( 'input' ).simulate( 'focus' );
+			expect( completersFilter ).toHaveBeenCalledWith( expectedCompleters );
+		} finally {
+			wrapper.unmount();


It'd be less code to put wrapper.unmount() inside afterEach call instead of using try / finally in all tests.

I was actually doing it that way initially but switched away because I thought I would be able to use shallow for most of the tests. I was wrong. :)

I started to prefer the explicit try/finally because it is easy to accidentally declare a local const wrapper that shadows the outer variable, but I just noticed we have a no-shadow lint rule that covers this case.

I'll switch it back.

gziolo · 2018-03-22T15:33:29Z

blocks/hooks/autocompleters.js

+ */
+import defaultCompleters from '../autocompleters';
+
+/**


As far as I understand what happens here, it seems like it would be enough to have only one filter registered for autocompleters: components.Autocomplete.completers.

Most of the logic that is inside this file is directly related to the Autocomplete component. Instead of adding filter on the component.Autocomplete, it is also possible to set default autocompleters using:

const setDefaultAutocompleters = () => [ ...defaultCompleters ]; addFilter( 'components.Autocomplete.completers', 'core/autocompleters/set-default-autocompleters', setDefaultAutocompleters );

I'm not sure I correctly understand your comment, but my reading is that this component wrapper may not be necessary. Is that correct?

The purpose of this module is to filter autocompleters passed to Autocomplete components and to provide default completers when none are provided. The filtering is done on-demand per Autocomplete component when focus first enters the component after receiving new completers via props.

Your comment made me realize it would be more idiomatic to provide default completers via a components.Autocomplete.completers filter, and I've made that update.

brandonpayton · 2018-03-27T00:39:24Z

I added the first version of a README to blocks/autocompleters and plan to follow up on review comments first thing tomorrow.

gziolo

I left a few more comments, which circle back to my latest comments. In addition, I would like to make sure we have a good strategy for rollout. We should do our best to inform plugin developers about breaking changes introduced.

gziolo · 2018-03-30T13:47:53Z

docs/extensibility.md

@@ -153,3 +153,86 @@ _Note:_ This filter must always be run on every page load, and not in your brows
 Extending the editor UI can be accomplished with the `registerPlugin` API, allowing you to define all your plugin's UI elements in one place.

 Refer to [the plugins module documentation](../plugins/) for more information.
+
+## Overriding autocompleters


There were some major updates to the extensibility docs. I think this should now land into its own file.

OK, I will move them in an autocompletion.md doc. Does that work for you?

It all depends if you decide to have it as block specific or component specific :)

Here is the document for Extending Blocks.

It might be also its own Extending Components document. Autocompletion might be too granular.

gziolo · 2018-03-30T14:03:14Z

components/autocomplete/index.js

@@ -3,6 +3,7 @@
 */
 import classnames from 'classnames';
 import { escapeRegExp, find, filter, map } from 'lodash';
+import 'element-closest';


wp.components.Autocomplete was exposed for some time. Is there any way to make it backward compatible for the next 2 release cycles? We should also add a deprecation message to give a warning about the changes.

We should be able to detect a difference in the completer objects and log a deprecation message then. Do we want to limit such messages or print for every completer object seen with the old interface?

In other cases, it was done per function/component occurrence. See #5833 for reference.

gziolo · 2018-03-30T14:06:19Z

blocks/autocompleters/README.md

@@ -0,0 +1,117 @@
+Autocompleters


I have a feeling that this should be documented next to the original Autocomplete component. It is also breaking change as far as I understand. We should provide some notes how to update code when someone was using wp.components.Autocomplete.

I tend to agree with you. I don't know why the completer interface should be documented within @wordpress/blocks and not with the wp.components.Autocomplete component. I plan to move the completer interface JSDoc into components/autocomplete/index.js. Sound reasonable?

The blocks completer probably belongs within @wordpress/blocks.

I feel better keeping the user completer within @wordpress/blocks as well because @wordpress/components seems to be more generic than the users module which actually hits the WP REST API.

We should provide some notes how to update code when someone was using wp.components.Autocomplete.

Where is a good place to provide such notes?

I plan to move the completer interface JSDoc into components/autocomplete/index.js. Sound reasonable?

Yes, this was my exactly my point. It's a general interface that should work with every custom Autocomplete component.

Where is a good place to provide such notes?

I think it is enough to include deprecated function in the code and leave a link to the new interface since it is now very well documented.

See also: https://github.com/WordPress/gutenberg/pull/5398/files#diff-cf74d2aaa31578636c008cace4de69f2L13.

gziolo · 2018-03-30T14:09:23Z

blocks/autocompleters/index.js

-		onSelect,
-	};
-}
+export default [ userAutocompleter ];


I'm not convinced that we should have the default setup exposed in here. We should rather set it in the hook.

Agreed. I think that is a better idea. I'll make a note to make the update once we decide what to do instead of the components.Autocomplete filter.

gziolo · 2018-03-30T14:19:40Z

blocks/hooks/autocompleters.js

+	return completers;
+}
+
+addFilter( 'components.Autocomplete', 'core/autocompleters', withFilteredAutocompleters );


I still don't understand why we need components.Autocomplete filter. It is applied globally so it wraps all autocompleters. Could it be applied directly in components/Autocomplete? Or should we alternatively create BlockAutocomplete component and use it inside RichText?

I don't believe there is anything here that requires the components.Autocomplete filter. I am trying to remember my frame of mind when I wrote this. In this comment, I may have thought you were suggesting we add this functionality separately via such a filter, so it is added when blocks are in use. Now that you mention it though, I suppose block and non-block UI may exist simultaneously so that we don't want to do this for all instances.

Should autocompletion extensibility be a feature limited to @wordpress/blocks or also supported for @wordpress/components?

My mind has been set on blocks, and I have not given much thought to WordPress UI outside of blocks. I don't yet know what I think about that question, but an answer will help us know what to do instead of this unnecessary filter.

If it is a blocks-only feature, I think creating a BlockAutocomplete component makes sense to provide the on-focus behavior currently added via the components.Autocomplete filter, and we should also use a blocks-related filter name instead of the generic components.Autocomplete.completers.

I'm thinking that the opinion of a default completers list should also be limited to blocks, but if we want autocompleters to be extensible for @wordpress/components as well, I am not sure how to filter and provide defaults only within a block context because the filter could be applied in both block and non-block contexts.

I went through a similar thinking process for MediaUpload component in #5846. I wanted to move it to components module, but the feedback I received suggests to leave it inside blocks module together with the accompanying filer. I think it makes a lot of sense because the idea is to have a set of independent components as @youknowriad explained:

I'm not sure we want to move components relying on filters to the components folder. I feel like the components folder is for generic React components while adding filters adds an unwanted dependency the wp hooks.

I would follow that advice and remove the components.Autocomplete filter and introduce BlockAutocomplete component which uses components.Autocomplete.completers internally.

gziolo · 2018-04-03T08:43:09Z

blocks/library/paragraph/test/__snapshots__/index.js.snap

-        />
-        <p
-          class="blocks-rich-text__tinymce wp-block-paragraph"
+  <div


So it looks like this issue with the obsolete div elements isn't something new. Let's leave it as it is and circle back later. We can investigate if React 16.3 has something to offer to mitigate it. https://reactjs.org/blog/2018/03/29/react-v-16-3.html - introduces createRef and forwardRef - they might be what we need in this case. We still need to expose those methods in a separate PR.

gziolo

I left another round of comments :)

gziolo · 2018-04-04T11:49:38Z

docs/extensibility/autocompletion.md

+
+// Adding the filter
+wp.hooks.addFilter(
+	'components.Autocomplete.completers',


Should be blocks not components :)

gziolo · 2018-04-04T11:49:43Z

docs/extensibility/autocompletion.md

+
+// Adding the filter
+wp.hooks.addFilter(
+	'components.Autocomplete.completers',


Should be blocks not components :)

gziolo · 2018-04-04T11:51:28Z

docs/extensibility/autocompletion.md

+```js
+// Our completer
+var acronymCompleter = {
+	name: 'acronyms',


Nit: Should we use acronym name to match with the variable name and description?

I've struggled with whether to make name singular or plural. It feels weird to call it "acronym" because it is not an acronym, it completes acronyms. I ended up using plural for the user and block completers and wanted to be consistent here. I'm fine changing it to singular everywhere though. Any thoughts?

Let’s keep it consistent. No need to update 👍

gziolo · 2018-04-04T11:55:01Z

components/autocomplete/completer-compat.js

+}
+
+export function toCompatibleCompleter( deprecatedCompleter ) {
+	deprecated( 'Original autocompleter interface', {


gziolo · 2018-04-04T12:03:34Z

components/autocomplete/completer-compat.js

+		compatibleCompleter.className = deprecatedCompleter.className;
+	}
+	if ( 'allowNode' in deprecatedCompleter ) {
+		compatibleCompleter.allowNode = ( ...args ) => deprecatedCompleter.allowNode( ...args );


Could you map other props like this?

const extraProps = [ 'className', 'allowNode', 'allowContext' ]. filter( key => key in deprecatedCompleter ). map( key => deprecatedCompleter[ key ]; const compatibleCompleter = { name: generateCompleterName(), // ... ...extraProps, };

It seems like those functions don't add any new logic.

Yep, that's more concise. I wanted to preserve completer context for the methods in that list, but Autocomplete already doesn't call them with completer context.

gziolo · 2018-04-04T12:14:51Z

blocks/autocompleters/index.js

-	};
-}
+export { default as blockAutocompleter } from './block';
+export { default as userAutocompleter } from './user';


Nice, thanks 👍

gziolo · 2018-04-04T12:17:08Z

components/autocomplete/README.md

+Autocomplete
+============
+
+TODO


It can be very brief - usually, one line is enough :)

Adding a one line description works for me. :) I was thinking I needed to document props for the Autocomplete component as well since the README didn't yet exist, but that doesn't have to be part of this PR.

Follow up is fine

gziolo · 2018-04-04T12:18:24Z

components/autocomplete/README.md

+		<abbr title={ option.name }>{ option.visual }</abbr>
+	),
+};
+```


Can we also add a line where it is wired into an actual component?

I created a simple example wiring to Autocomplete wrapping an input element, but it didn't work. It seems like Autocomplete is assuming a contenteditable target, since it inserts HTML rather than simple text. I'll play with a simple contenteditable example but don't have anything yet.

gziolo · 2018-04-04T12:18:53Z

blocks/autocompleters/README.md

+Autocompleters
+==============
+
+Autocompleters enable us to offer users options for completing text input. For example, Gutenberg includes a user autocompleter that provides a list of user names and completes a selection with a user mention like `@mary`.


Can we add a reference to @wordpress/components? It contains the same content and will be difficult to keep in sync otherwise.

I actually meant to remove that entirely after moving the content but missed doing so last night.

Leaving a README with a reference to @wordpress/components seems good. Is there a better way than direct GitHub links to link to component READMEs?

Unfortunately not yet :(

gziolo · 2018-04-04T12:26:00Z

blocks/autocomplete/index.js

+			const hasFocus = this.parentNode.contains( document.activeElement );
+
+			// This may trigger another render but only when the component has focus.
+			if ( hasFocus && this.hasStaleCompleters() ) {


Can you explain why we need this additional check in here? It seems like having onFocus handler should be enough.

It is technically possible for the completers prop to change while the component contains the focus. This check exists for that case, since onFocus will already have been called. I don't think that is likely to occur, but I'd rather just cover the case. Sound OK?

I hope it’s good 😃It makes sense, might make sense to leave an inline comment.

It's mostly about not assuming when props will be updated. Sure, I can add a comment. :)

gziolo · 2018-04-04T12:40:24Z

I noticed that this PR introduces regression for the following use cases:

Image and Gallery - it is not possible to update the caption field. You can edit pre-existing text though.
Quote and Pullquote - it is not possible to type in the cite part.

brandonpayton · 2018-04-04T14:21:30Z

Thanks for the last round of comments. I'm going through them and will look into the regressions. I'd noticed some awkwardness focusing the cite input but thought it was unrelated to this PR. I'm glad you caught it.

brandonpayton · 2018-04-04T19:00:06Z

I fixed the regressions and made updates in response to review feedback.

I'd like to add a couple more automated tests, but this is ready for another round of manual testing.

mkaz · 2018-04-04T21:05:52Z

Looking good, I've tested it out and auto complete is working in lists, captions, quotes, heading and I assume other RichText components. Nice work!

gziolo · 2018-04-05T09:42:41Z

components/autocomplete/completer-compat.js

+		link: 'https://github.com/WordPress/gutenberg/blob/master/components/autocomplete/README.md',
+	} );
+
+	const optionalProperties = [ 'className', 'allowNode', 'allowContext' ]


It needs to be converted to an object, my fault :)

I did the same thing. It's funny how our minds can just fill in the gaps sometimes. Thanks for fixing it.

gziolo · 2018-04-05T09:55:54Z

I'm seeing the following error once per 20ish focus changes:

Uncaught TypeError: Cannot read property 'top' of undefined
at RichText.getFocusPosition (index.js:437)
at RichText.onNodeChange (index.js:635)
at c.i [as fire] (tinymce.min.a3a2e6f1.js:8)
at w.fire (tinymce.min.a3a2e6f1.js:8)
at Object.nodeChanged (tinymce.min.a3a2e6f1.js:11)
at w.nodeChanged (tinymce.min.a3a2e6f1.js:12)
at w. (tinymce.min.a3a2e6f1.js:11)
at c.i [as fire] (tinymce.min.a3a2e6f1.js:8)
at w.fire (tinymce.min.a3a2e6f1.js:8)
at x.g (tinymce.min.a3a2e6f1.js:8)

It doesn't break anything, but it would be nice to make sure it isn't triggered.

gziolo · 2018-04-05T10:03:38Z

Tried with the legacy user completer and it shows a very nice message:

Nice job in this area 👍

gziolo

We went through so many iterations and it looks really solid. I tested it quite long today and didn't find any issues. It is a really great improvement 👍

I made sure that deprecation message is printed to the console when using old autocomplete intereface.

I also tested acronym completer example applied with hook. It works like a charm. It even gets applied just after I install it using JS console.

Great work. There are some random errors that might be not related to this PR, but it would be nice if you would try to reproduce them.

Time to merge 🎉

brandonpayton · 2018-04-05T18:14:42Z

Great work. There are some random errors that might be not related to this PR, but it would be nice if you would try to reproduce them.

Sure, I'll play with this to see what I can find.

Thanks for fixing the silly mistake with array spread instead of object spread in the completer compat module. Also thank you for all the time you gave reviewing and testing this. 🙇

aduth self-requested a review January 24, 2018 18:36

brandonpayton force-pushed the try/overriding-autocomplete-with-filters branch from 91b616e to 72d8371 Compare January 24, 2018 18:48

gziolo reviewed Jan 31, 2018

View reviewed changes

gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Jan 31, 2018

gziolo requested a review from noisysocks February 1, 2018 08:30

brandonpayton mentioned this pull request Feb 1, 2018

Add reusable blocks to the block autocompleter #4769

Closed

brandonpayton force-pushed the try/overriding-autocomplete-with-filters branch from 72d8371 to 3b17dec Compare February 7, 2018 19:49

gziolo reviewed Feb 12, 2018

View reviewed changes

brandonpayton force-pushed the try/overriding-autocomplete-with-filters branch from bf0c6ce to e3277e1 Compare March 21, 2018 18:47

gziolo reviewed Mar 22, 2018

View reviewed changes

brandonpayton force-pushed the try/overriding-autocomplete-with-filters branch from 1661d2a to 5c637d0 Compare March 28, 2018 13:56

gziolo added this to the 2.6 milestone Mar 29, 2018

gziolo added [Type] Enhancement A suggestion for improvement. [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Mar 29, 2018

gziolo reviewed Mar 30, 2018

View reviewed changes

brandonpayton force-pushed the try/overriding-autocomplete-with-filters branch from 5c637d0 to 2bc0bf5 Compare April 3, 2018 06:32

gziolo reviewed Apr 3, 2018

View reviewed changes

gziolo reviewed Apr 4, 2018

View reviewed changes

brandonpayton force-pushed the try/overriding-autocomplete-with-filters branch from 36c1585 to 6174f58 Compare April 4, 2018 18:53

brandonpayton force-pushed the try/overriding-autocomplete-with-filters branch from 6174f58 to 70f9eb4 Compare April 4, 2018 20:48

Support overriding autocomplete with filters

c48f351

brandonpayton force-pushed the try/overriding-autocomplete-with-filters branch from 70f9eb4 to c48f351 Compare April 4, 2018 23:17

gziolo reviewed Apr 5, 2018

View reviewed changes

Components: Small tweak to the Autocomplete component

875f6dc

gziolo assigned brandonpayton Apr 5, 2018

gziolo approved these changes Apr 5, 2018

View reviewed changes

gziolo merged commit 9958d2c into WordPress:master Apr 5, 2018

Declare and override autocompleters via filter #4609

Declare and override autocompleters via filter #4609

Conversation

brandonpayton commented Jan 20, 2018 • edited Loading

Description

How Has This Been Tested?

Types of changes

Checklist:

brandonpayton commented Jan 24, 2018

Choose a reason for hiding this comment

gziolo commented Jan 31, 2018

brandonpayton commented Feb 1, 2018

gziolo commented Feb 1, 2018 • edited Loading

brandonpayton commented Feb 3, 2018 • edited Loading

brandonpayton commented Feb 5, 2018

gziolo commented Feb 6, 2018

brandonpayton commented Feb 6, 2018

brandonpayton commented Feb 7, 2018 • edited Loading

brandonpayton commented Feb 9, 2018

brandonpayton commented Feb 10, 2018 • edited Loading

brandonpayton commented Feb 12, 2018 • edited Loading

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

gziolo Feb 12, 2018 • edited Loading

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

Choose a reason for hiding this comment

brandonpayton Feb 12, 2018 • edited Loading

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

Choose a reason for hiding this comment

brandonpayton commented Mar 21, 2018

pento commented Mar 21, 2018

gziolo 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

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

brandonpayton commented Mar 27, 2018

gziolo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Apr 3, 2018 • edited Loading

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

brandonpayton commented Jan 20, 2018 •

edited

Loading

gziolo commented Feb 1, 2018 •

edited

Loading

brandonpayton commented Feb 3, 2018 •

edited

Loading

brandonpayton commented Feb 7, 2018 •

edited

Loading

brandonpayton commented Feb 10, 2018 •

edited

Loading

brandonpayton commented Feb 12, 2018 •

edited

Loading

gziolo Feb 12, 2018 •

edited

Loading

brandonpayton Feb 12, 2018 •

edited

Loading

gziolo Apr 3, 2018 •

edited

Loading