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

Add shared blocks to the blocks autocompleter #6067

Merged
merged 2 commits into from
May 29, 2018

Conversation

brandonpayton
Copy link
Member

Description

This is a PR to add shared blocks to the blocks autocompleter.

It is functional but needs to be able to trigger and wait for retrieval of shared blocks. Currently it just uses what is in the core/editor store which doesn't include shared blocks until the inserter menu is opened and triggers a request for shared blocks.

How Has This Been Tested?

I've tested this manually so far but intend to write automated tests.

Screenshots (jpeg or gifs if applicable):

shared-block-autocomplete

Types of changes

Adds a blocks.Autocomplete.completers filter that enhances the blocks completer with one that includes and supports shared blocks.

Checklist:

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

@brandonpayton brandonpayton added the [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) label Apr 9, 2018
@brandonpayton brandonpayton requested a review from noisysocks April 9, 2018 00:16

addFilter(
'blocks.Autocomplete.completers',
'core/edit-post/blocks/media-upload/replaceMediaUpload',
Copy link
Contributor

Choose a reason for hiding this comment

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

The name will probably need changing, as this name doesn't seem to match the context of this filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is embarrassing... I was trying to get something out there quickly and copied something nearby.

Copy link
Member Author

Choose a reason for hiding this comment

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

... coding while watching my kids. Thanks for the heads up.

@brandonpayton brandonpayton force-pushed the add/reusable-blocks-autocompletion branch 3 times, most recently from 6973ae0 to 11b2a9e Compare April 9, 2018 01:48
@noisysocks
Copy link
Member

It is functional but needs to be able to trigger and wait for retrieval of shared blocks.

I think this should be fairly straightforward—we just gotta dispatch fetchSharedBlocks when the autocomplete UI mounts. This is what the inserter does.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

😍


// Export for unit test
export function addReusableBlocksCompletion( completers ) {
const blocksCompleter = completers.find( c => 'blocks' === c.name );
Copy link
Member

Choose a reason for hiding this comment

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

We should use Lodash's find method as Array.prototype.find is not supported in IE11 and we do not currently polyfill it.

Copy link
Member Author

Choose a reason for hiding this comment

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

✅ will do. Thanks for the heads up. I'd assumed a polyfill.

return isInitialQuery ?
// Before we have a query, offer frecent blocks as a sensible default.
editor.getFrecentInserterItems() :
editor.getInserterItems();
Copy link
Member

Choose a reason for hiding this comment

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

Will this list of items stay current when shared blocks are added and deleted? Just checking since usually we use withSelect which automatically subscribes us to store updates, but in this case we are calling select manually.

Also, we will need to pass the enabledBlockTypes argument to these selectors so that blocks that have been disabled via the allowed_block_types filter cannot be inserted into the post. Usually, we get this setting by using withContext, but I'm not sure how that translates to this since we're not working with a 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.

A completer provides a static list of options, but Autocomplete will request a new list for each keystroke. If a completer's options property is a function, Autocomplete will call it to get the next options, and those options can be based on updated data like this.

Also, we will need to pass the enabledBlockTypes argument to these selectors so that blocks that have been disabled via the allowed_block_types filter cannot be inserted into the post.

I wondered about this but noticed that the default for enabledBlockTypes is true for those functions so we are getting the desired behavior by default. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

I wondered about this but noticed that the default for enabledBlockTypes is true for those functions so we are getting the desired behavior by default. Am I missing something?

Yes, the idea is that the caller of getInserterItems() should pass along an array of allowed block types so that plugin developers can turn specific block types on or off via a filter.

(I regret making the argument optional.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for explaining.

Usually, we get this setting by using withContext, but I'm not sure how that translates to this since we're not working with a component.

This could be interesting, but it's good we're feeling this friction now. I'll see what I can find.

Copy link
Member

@noisysocks noisysocks Apr 9, 2018

Choose a reason for hiding this comment

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

It turns out that this issue exists in master, so feel free to ignore it for now. I opened #6070 to track it.

@brandonpayton
Copy link
Member Author

brandonpayton commented Apr 9, 2018

I think this should be fairly straightforward—we just gotta dispatch fetchSharedBlocks when the autocomplete UI mounts. This is what the inserter does.

I would prefer we just make that request and have the menu naturally update because updated state flows to the menu, but menus from the Autocomplete component don't work that way (though we could explore this later). Autocomplete gets a list of options from the completer and creates UI based on those options. The options aren't updated until a new set of options is retrieved for the next key stroke. I hope my explanation makes sense.

I'm seeing two choices:

  1. Return a promise that waits on the fetch to complete.
    • Pro: Fresh data
    • Con: High-latency UX. The completer menu won't display quickly.
  2. Dispatch the fetch action but immediately respond with options based on the current state.
    • Pro: The completer menu will display quickly with blocks we know about.
    • Con: Shared blocks won't immediately show in the menu.

I'm personally leaning toward the second as we could dispatch the fetch when the completers filter runs to give us a better chance of the data being available ahead of time.

Any thoughts on this, or other ideas?

@noisysocks
Copy link
Member

Any thoughts on this, or other ideas?

Option 2 sounds OK to me. We could also look at eagerly dispatching fetchSharedBlocks() when the editor initialises instead of lazily when the inserter/autocompleter mounts. I don't think it's necessary that shared blocks that have been created since the editor was launched appear in the inserter/autocompleter.

I imagine that our solution to the enabledBlockTypes thing I brought up in #6067 (comment) might influence what we do here, e.g. if we need to introduce a component somewhere.

@brandonpayton
Copy link
Member Author

brandonpayton commented Apr 9, 2018

I fixed the dependency on Array.prototype.find and went with option 2 for now. This seems like a reasonable first implementation.

It's not looking very unit testable, due to being entangled with shared state via select and createBlock. I'll leave the question of how to fix that for tomorrow. Thanks for your quick feedback!

@noisysocks
Copy link
Member

Meta note: this would fix #3791 and #4225. Possibly #6070 too if we can get that argument passed to getInserterItems 😄


// Export for unit test
export function addReusableBlocksCompletion( completers ) {
const blocksCompleter = find( completers, c => 'blocks' === c.name );
Copy link
Member

Choose a reason for hiding this comment

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

Can it be expressed with destructuring to avoid using a meaningful name c?

const blocksCompleter = find( completers, ( { name } ) => 'blocks' === name );

blocksCompleter.getOptionCompletion = getBlockCompletion;

// Fetch shared block data so it will be available to this completer later.
dispatch( 'core/editor' ).fetchSharedBlocks();
Copy link
Member

@gziolo gziolo Apr 9, 2018

Choose a reason for hiding this comment

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

I think it's okey as a quick temporary solution. However, in the long run we should avoid introducing this kind of changes and refactor fetchSharedBlocks to be hidden behind new wp.data API. It's a perfect fit to be implemented as resolver. See: https://github.com/WordPress/gutenberg/tree/master/data#registering-a-store. With that in place, such network request would be executed when trying to get list of shared blocks automatically.

Copy link
Member

Choose a reason for hiding this comment

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

It might be hard to refactor shared blocks to use wp.data now that its implementation is pretty tied up with how blocks are stored in redux (see #5228).

Copy link
Member

Choose a reason for hiding this comment

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

@aduth or @youknowriad can you confirm that wp.data doesn't make sense in this context?

Copy link
Member

Choose a reason for hiding this comment

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

There might be a misunderstanding here: wp.data is effectively an abstraction over Redux. I don't think the two are incompatible, and I'm inclined to agree that triggering a fetch here is not the best approach.

@gziolo
Copy link
Member

gziolo commented Apr 9, 2018

#6067 (comment) - I'm wondering if we really need to use filters to trigger network request. Maybe it would be enough to refactor getInserterItems selector to use side-effects with the resolver implementation. It might simplify the logic also in other places where fetchSharedBlocks is called to make sure that the list of shared blocks is populated.

On the UX level, I'm not convinced that we need to provide a different set of blocks when data is not there. How do we handle it for users completer?

@brandonpayton
Copy link
Member Author

@gziolo That's an interesting idea about a getInserterItems side-effect. 🤔

On the UX level, I'm not convinced that we need to provide a different set of blocks when data is not there. How do we handle it for users completer?

The users completer returns a promise for options and caches the promise so we don't continue requesting. There is a noticeable delay before the user completion menu first displays. It's enough that I've had it in the back of my mind that we may want to show some sort of indicator that we're waiting to display the completion menu.

@brandonpayton
Copy link
Member Author

brandonpayton commented Apr 16, 2018

This is an interesting data flow challenge. The completer now needs contextual information to work (allowedBlockTypes), but autocompleters aren't React elements that have access to React context. At first, I considered whether we should provide contextual information to completers via a second argument like options( query, completerContext ), but I don't think we can provide sufficient context to meet the needs of all future completers. Instead, since this PR overrides the block completer options for the editor, I'm wondering whether the editor store(s) have sufficient information to determine what the currently allowed block types are.

I have to disconnect for the day but am planning to look at this tomorrow. If you know whether a store contains sufficient info for this, please feel free to leave a comment. :) Thanks!

@noisysocks
Copy link
Member

noisysocks commented Apr 17, 2018

I'm tempted to suggest that we just move editor settings out of context and into a regular global variable.

Context is useful for passing dynamic data to deeply nested components, but this data: a) is static; and b) needs to be accessed by non-components.

Ordinarily one stores static configuration in a constant (e.g. const EDITOR_SETINGS = { ... }) but since PHP generates this configuration the next best thing is a global variable (e.g. window._wpEditorSettings = { ... }).

(Aside: We already have quite a few global configuration variables—we should probably look at consolidating them all into one this is all the stuff that came from PHP object.)

@brandonpayton
Copy link
Member Author

Thanks for your thoughts, @noisysocks. Is there a reason the settings shouldn't be in the editor store? Offhand, it seems like a natural place for them to go.

Regarding allowedBlockTypes, I'd thought that allowedBlockTypes could vary per block and maybe even be dictated by a client-side filter that reacted to block type or something. Is any of that the case or possibly where we are heading?

Either way, I'm wondering whether the editor store should reflect that information. (I still need to look at what it currently includes)

@noisysocks
Copy link
Member

Is there a reason the settings shouldn't be in the editor store?

In my view, a Redux store is for data that is shared (✓) and changes (e.g. from user input) during the lifetime of the application (𝘅).

Regarding allowedBlockTypes, I'd thought that allowedBlockTypes could vary per block and maybe even be dictated by a client-side filter that reacted to block type or something. Is any of that the case or possibly where we are heading?

You're right. #5452 and #5448 changes how this works quite a bit. I'll need to think about this some more... 🤔

@brandonpayton
Copy link
Member Author

In my view, a Redux store is for data that is shared (✓) and changes (e.g. from user input) during the lifetime of the application (𝘅).

If you regard a data store more generally as a source of truth, you can reference that truth in one place, and later, if static data becomes dynamic data, the consumer doesn't have to worry. It's always referencing what the application says is true.

I'm not passionate about this case. I was just thinking it might be preferable to creating a new global and new dependencies on that global.

@brandonpayton
Copy link
Member Author

I'll try to think about this more too. It's funny. I thought this was going to be a quick change. 😄

@aduth
Copy link
Member

aduth commented Apr 18, 2018

In my view, a Redux store is for data that is shared (✓) and changes (e.g. from user input) during the lifetime of the application (𝘅).

There's some truth to this with the data module. I've been a bit saddened that we've drifted away to considering the context for how these stores integrate into a React application. In my mind, ideally the editor component creates an umbrella of context from which descendants can read, but not necessarily that there's only a single source of truth; particularly for how the editor might integrate into a larger application like Calypso, or use-cases where a developer may want to have several instances of an editor present on the same page. It's not all bad though, the data module certainly dramatically simplifies usage, though it really engrains this idea that there's "one source", and is not too much different than a global.

To your point on consolidation globals, I very much agree. It shouldn't be such a challenge to initialize an editor in a non-WordPress setting. There's been a few prior discussions here, with @youknowriad's https://github.com/youknowriad/standalone-gutenberg and projects like https://www.npmjs.com/package/@frontkom/gutenberg (discussed in Slack).

@aduth
Copy link
Member

aduth commented Apr 18, 2018

FWIW, ship may have already sailed, but taking a glance here makes me wish extending the autocompleter was more integrated into the React component hierarchy, so that we weren't needing to have to deal with issues of differing approaches for loading in data vs. what already exists in other components.

Related to this, I've been playing a bit with creating some sort of InserterContext component to encapsulate some of the behaviors of "allowed blocks" and automating parts of rootUID, layout, etc.

@brandonpayton
Copy link
Member Author

FWIW, ship may have already sailed, but taking a glance here makes me wish extending the autocompleter was more integrated into the React component hierarchy, so that we weren't needing to have to deal with issues of differing approaches for loading in data vs. what already exists in other components.

This is a pain I felt while thinking about autocompletion extensibility. I appreciate your comment because this is something we'll be dealing with down the line. It would be painful to change this now, but now seems a better time than never.

I don't know the reasoning behind the original approach to completers, but before we limp along with completer-specific data flow, I'd like to give some thought to how completers could be treated more like components and enjoy the same data flow as everything else. I think the challenge is that there are some aspects that should be owned by the Autocomplete component and others by a completer component. For example, selecting a completer from the list is naturally owned by Autocomplete, but satisfying data dependencies and rendering should probably be owned by a completer component.

Maybe we won't want to change anything, but I'm going to think a bit before finding a way to proceed with what we have.

@brandonpayton
Copy link
Member Author

I just realized that saying I'll go think about this may discourage others from getting involved.
💡 I'd love any help, ideas, or commentary you have.

I thought about this a lot today and haven't discovered a component-based approach that is as clean as we have with today's completers. Beyond a component-based approach, I'm split between:

  1. Do nothing for now and try to provide what we need for the block completer via closure.
  2. Add a completerContext argument to all completer functions. I'm concerned with how we'll decide what belongs in completerContext and think we'd probably end up adding a filter for extending it. Maybe that's OK, but I'm wary of the added complexity.

@noisysocks
Copy link
Member

Related to this, I've been playing a bit with creating some sort of InserterContext component to encapsulate some of the behaviors of "allowed blocks" and automating parts of rootUID, layout, etc.

Curious to hear more about this 🙂

@aduth
Copy link
Member

aduth commented Apr 23, 2018

On second thought, maybe we don't need this to strictly live within a React component hierarchy, as part of the point of resolvers in the data module is that it simply ties a behavior to the selection of data, regardless of whether that selection is via select or a component's withSelect. So, ultimately, I don't think we want an explicit fetch to occur within the autocompleters, but the call to the selector which itself triggers the resolver to incur the fetch may be reasonable.

@aduth
Copy link
Member

aduth commented Apr 23, 2018

Curious to hear more about this 🙂

The rough idea would be that a component creates an umbrella, either via context or focus event handling (+ store state), where all components within wouldn't call the store's insertBlocks directly but rather the a context-provided "enhanced" inserter callback:

<Root>
    <Block>
        <InserterContextProvider rootUid="...">
            <Column>
                <InserterContextProvider layout="column-1">
                    <InserterContextConsumer>
                        { ( { onInsert } ) => (
                            <DropZone onInsert={ ( ...args ) => {
                                const maybeError = onInsert( ...args );
                                // handle error (unallowed block, etc)
                            } } />
                        ) }
                    </InserterContextConsumer>
                    <InserterContextConsumer>
                        { ( { onInsert, allowedBlockTypes } ) => (
                            <Inserter onInsert={ onInsert } options={ allowedBlockTypes } />
                        ) }
                    </InserterContextConsumer>
                </InserterContextProvider>
            </Column>
        </InserterContextProvider>
    </Block>
</Root>

Playing with aggregating context: https://codepen.io/aduth/pen/bvXvQO

@brandonpayton brandonpayton force-pushed the add/reusable-blocks-autocompletion branch from f088761 to 6b2afa6 Compare May 15, 2018 05:03
@brandonpayton
Copy link
Member Author

I see that I added this hook under edit-post for some reason but think it makes more sense in editor. I plan to move it.

Just noticed: The blocks completer now lives within editor, so we can just update the completer directly.

@gziolo
Copy link
Member

gziolo commented May 15, 2018

Either way, I'm happy to create a PR to make a getSharedBlocks resolver for the core/editor store and reduce direct use of the fetchSharedBlocks action.

That would be a good improvement in the follow-up PR. 👍
Let's get this one in and do the rest afterward. I only wanted to raise the awareness of the general direction where we are heading with everything that relates to network requests.

@brandonpayton brandonpayton force-pushed the add/reusable-blocks-autocompletion branch from 6b2afa6 to 8c6510f Compare May 15, 2018 23:07
@brandonpayton
Copy link
Member Author

I moved the changes directly into the editor's block completer but now need to:

  1. Fix the unit tests since the options now come from the store.
  2. Determine a good place to trigger shared block retrieval since that was being done in the hook.

@brandonpayton
Copy link
Member Author

I haven't had time to touch this but want to note my plan:

Provide a function to create the block completer that allows injecting block selector functions. This will allow us to stub the selector functions for unit test rather than requiring the actual editor store. The creation function will also give us a place to conditionally dispatch fetchSharedBlocks when shared blocks are not yet loaded in the store.

@brandonpayton brandonpayton force-pushed the add/reusable-blocks-autocompletion branch 2 times, most recently from 14aa79d to 533b299 Compare May 18, 2018 22:48
@brandonpayton
Copy link
Member Author

Unit tests are now fixed. There is a remaining TODO that can be resolved once #6753 is merged since its update to getInserterItems simplifies arguments and returns a sorted list by default.

I ended up compromising and triggering shared block fetch when a block completer is added by the default completers hook. It's less than ideal, but it...

  1. Is clearly marked as a hack.
  2. Can be removed once there is a way for the block completer to return a Promise for options while resolving its shared blocks dependency.

@noisysocks noisysocks force-pushed the add/reusable-blocks-autocompletion branch from 533b299 to b96662a Compare May 28, 2018 01:59
@noisysocks
Copy link
Member

Unit tests are now fixed. There is a remaining TODO that can be resolved once #6753 is merged since its update to getInserterItems simplifies arguments and returns a sorted list by default.

#6753 is merged now, so took the liberty of making this change in b96662a0a.

Everything's working great for me. I can insert shared blocks using the / command and it's super cool that the autocompleter now respects allowedBlockTypes and parent:

screen shot 2018-05-28 at 11 57 32

I'm fine with the fetchSharedBlocks() hack. I'll be looking into making fetchSharedBlocks a resolver soon, which will fix the awkwardness of dispatching this action in both the autocompleter and in the inserter menu.

Note that this PR closes #3791, #4225, #6070, and probably some others.

👍 :shipit: let's merge it.

@noisysocks
Copy link
Member

noisysocks commented May 28, 2018

Not sure why the managing-links E2E test is failing. It seems unrelated. I suspect #6971 may help. Update: Yep, it did.

brandonpayton and others added 2 commits May 28, 2018 13:57
This updates the blocks completer to use the editor data store so only
supported blocks will be offered as completion options. In addition to
respecting the supported blocks list, shared blocks are now included as
completion options.
Use only getInserterItems() in the blocks autocompleter. This selector
will do all of the required filtering.
@noisysocks noisysocks force-pushed the add/reusable-blocks-autocompletion branch from b96662a to 2a2e247 Compare May 28, 2018 04:02
@brandonpayton
Copy link
Member Author

#6753 is merged now, so took the liberty of making this change in b96662a.

Thank you, @noisysocks!

I'm fine with the fetchSharedBlocks() hack. I'll be looking into making fetchSharedBlocks a resolver soon, which will fix the awkwardness of dispatching this action in both the autocompleter and in the inserter menu.

A resolver would be good, but I want to note that it won't solve the issue for autocompleters because of the way we originally designed completers to provide static lists. If the data isn't there when we initially select it, then it won't be there in the first display of the block completion menu. We may still find value in triggering prefetch somehow. (If completers could return an Observable of options for a given query, it would be a different story : )

@brandonpayton
Copy link
Member Author

A resolver would be good, but I want to note that it won't solve the issue for autocompleters because of the way we originally designed completers to provide static lists. If the data isn't there when we initially select it, then it won't be there in the first display of the block completion menu. We may still find value in triggering prefetch somehow.

Actually, even the way we trigger prefetch here is not early enough to give me shared blocks when I click the initial paragraph and type '/'. My not-too-fast, casual usage beats the request for shared blocks in my local environment. Relying on a resolver wouldn't be much different.

The rest of the block types are available when the editor is loaded. It seems odd that shared blocks aren't loaded in the editor at the start or at least prefetched. Is there a reason we shouldn't do that?

NOTE: I'm testing locally and plan to merge this morning.

@brandonpayton brandonpayton merged commit 47449be into master May 29, 2018
@youknowriad youknowriad deleted the add/reusable-blocks-autocompletion branch May 29, 2018 14:57
@noisysocks
Copy link
Member

The rest of the block types are available when the editor is loaded. It seems odd that shared blocks aren't loaded in the editor at the start or at least prefetched. Is there a reason we shouldn't do that?

Nope! 😄

@mtias
Copy link
Member

mtias commented Jun 4, 2018

Thanks for getting this one in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants