-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 reusable blocks to the block autocompleter #4769
Conversation
return ( | ||
<Autocomplete completers={ [ blockAutocompleter( { items, onReplace } ), userAutocompleter() ] }> | ||
{ ( { isExpanded, listBoxId, activeId } ) => ( | ||
// TODO: These aria attributes probably belong on the <Editable>... not sure how to make that happen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only problem I ran into. Not sure where these aria attributes which were previously on the <Editable>
should go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that they fit better here than as part of <Editable />
. I think @afercia might be better suited to answer this question.
const { name, items, onReplace } = props; | ||
|
||
// Only wrap paragraph blocks with <Autocomplete> | ||
if ( name !== 'core/paragraph' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes a lot of sense for blockAutocompleter
to be applied to a paragraph block only, but not sure if we want to keep this limitation for userAutocompleter
, too.
We could make it more customizable by using supports proprty from the block definition. One way of doing it would be:
name: 'core/paragraph',
supports: {
autocompleters: [ 'block', 'user' ]
},
name: 'core/list',
supports: {
autocompleters: [ 'user' ]
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting idea, @gziolo. It's better to declare autocompleters explicitly than to bury them in block implementation.
Would it be better to provide the actual completers instead of strings? Do we gain anything from the indirection of using strings?
If we specify an actual list of completers, we could override aspects of them using the blocks.registerBlockType
filter rather than the dedicated filter discussed by #4609. The initial filters I proposed there were too granular for a first pass, but maybe using blocks.registerBlockType
for this isn't granular enough. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to provide the actual completers instead of strings? Do we gain anything from the indirection of using strings?
Yes, this is also an option to define them as part of the block API. I like how this discussion goes. Very good ideas :)
We do something similar with transformers
. See here:
gutenberg/blocks/library/code/index.js
Line 37 in 727da2c
transforms: { |
blocks.registerBlockType
as described here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @aduth @youknowriad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to add autocompleters more broadly was one of the initial reasons for the refactoring of these autocompleters. so yes 💯
I like thesupports
API thought as it can be easily moved to the backend for instance and I see other blocks using the "block" autocompleter as well (for example heading).
Edit: The problem about the supports API (or any generic API) is that it assumes that it would work regardless of the block implementation. I'm not certain it's the case. What about blocks with multiple RichText
components or without any input/textarea/richtext? How will this behave in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I comment out this check then it seems to work OK with blocks that have multiple RichText
components:
And it doesn't seem to affect blocks that use <input>
s and <textarea
>s:
So I think it would be reasonable to to have all blocks wrapped with the user autocompleter and only paragraph blocks wrapped with the block autocompleter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think it would be reasonable to to have all blocks wrapped with the user autocompleter and only paragraph blocks wrapped with the block autocompleter.
Interesting approach. I like the idea of having more specialized HOCs 👍
import { blockAutocompleter, userAutocompleter } from './autocompleters'; | ||
import { getInserterItems } from '../../store/selectors'; | ||
|
||
function withAutocomplete( BlockEdit ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this approach using a HOC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 😃
enabledBlockTypes: settings.blockTypes, | ||
} ) ), | ||
connect( ( state, ownProps ) => ( { | ||
items: getInserterItems( state, ownProps.enabledBlockTypes ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As part of integrating Gutenberg into Automattic P2's, we need to use a different data source for the users autocompleter because we allow pinging any Automattician, not just users who are a member of a particular P2. I'm not too familiar with how data dependencies are satisfied within Gutenberg (except my perception is that there are a number of approaches and that withApiData
is preferred for the future). Do you see a good way for us to override the request and provision of completion data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't read well enough :). I see now that only blockAutocompleter
is initialized with a list of items. Other completers like the userAutocompleter
could source their own data if they want.
Pulls <Autocomplete> out of the paragraph block and implements block autocompletion via a BlockEdit filter. By adding the filter in the `editor` module, we can easily add reusable blocks into the autocompletion since we have access to editor's Redux state.
11bf8ff
to
e576267
Compare
Closing due to staleness. Will revisit this soon. |
Would fix #4769 and fix #4225.
Proof of concept of the third approach listed in #3791 (comment).
Pulls
<Autocomplete>
out of theparagraph
block and implements block autocompletion via aBlockEdit
filter.By defining the filter in the
editor
module, we can easily add reusable blocks into the autocompleter since we can access to theeditor
Redux state.cc. @aduth @youknowriad