Navigation: use same default implementation of __experimentalFetchLinkSuggestions in post, site, navigation, widget editor #29993
Conversation
gwwar
requested review from
adamziel,
draganescu,
ellatrix,
kevin940726,
noisysocks,
talldan and
tellthemachines
as code owners
|
Size Change: -551 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
Thanks for cleaning up this bit of technical debt!
The problem with having We had this exact problem with We could do something similar to that and move cc. @youknowriad |
|
I vote for something line |
|
I don't like |
gwwar
force-pushed
the
fix/navigation-link-search
branch
from
ea63c67
to
17cb5aa
Compare
packages/wordpress-block-editor-config/src/settings/__experimental-fetch-link-suggestions.js
Outdated
Show resolved
Hide resolved
gwwar
force-pushed
the
fix/navigation-link-search
branch
2 times, most recently
from
7e4b4f9
to
e867cff
Compare
packages/wordpress-block-editor-config/src/settings/__experimental-fetch-link-suggestions.js
Outdated
Show resolved
Hide resolved
packages/wordpress-block-editor-config/src/settings/__experimental-fetch-link-suggestions.js
Outdated
Show resolved
Hide resolved
packages/wordpress-block-editor-config/src/settings/__experimental-fetch-link-suggestions.js
Outdated
Show resolved
Hide resolved
gwwar
force-pushed
the
fix/navigation-link-search
branch
from
dcdb114
to
9cfa345
Compare
|
@youknowriad @draganescu @noisysocks this one is ready for another 👀 on package setup feedback. Were there any other common settings we'd like to pull in, or is it okay that we're adding a single experimental value? |
|
I think there's the uploadMedia handler that could go into this package as well? |
There is
Does it really need to be another package? I also don't think it's specific to the block editor so the currently proposed name is misleading (more details about editor packages: https://github.com/WordPress/gutenberg/blob/trunk/docs/explanations/architecture/modularity.md#editor-packages). All the screens that consume this functionality depend on
The rationale behind that was to be able to reuse the functionality inside the core blocks. Although, now I see that it doesn't apply to |
Is this true? For me the "editor" package has always been about the "post editor" so it doesn't sound like a great fit for me. |
You're right when you say that these two packages have been extracted for the same reason as the link handler. But that reason is not to be able to use them in the block library for me, it's because these are WP specific concepts that are not specific to the post editor. |
gutenberg/packages/edit-site/package.json Line 37 in 021864d |
|
Should I spin up an alternative PR to sync up the site-editor search implementation, while we 🤔 about where to put common WP block-editor settings? |
|
I think we should just make a decision. what do people think about putting this in "@wordpress/core-data". In a way it's our official API to the REST API. Even if this handler is not tied to the store, it might make sense to include there, maybe experimental to start with? |
|
It sounds to me like there could be a superset of The dependency tree would then look like this: That said, I'm also fine with putting |
|
To unblock this work you can start with using the
|
|
Sounds good I can try |
gwwar
force-pushed
the
fix/navigation-link-search
branch
from
9cfa345
to
3f37473
Compare
|
✅ This is ready for review. It still feels a bit out of place in No updates to the README for now, since this function is still experimental |
| * @return { WPFetchLinkSuggestions } Function that fetches link suggestions | ||
| * | ||
| */ | ||
| const createFetchLinkSuggestions = ( { |
There was a problem hiding this comment.
Why have the higher order createFetchLinkSuggestions and not fetchLinkSuggestions as before? I think fetchLinkSuggestions is an easier API for consumers to understand and use. Also, having a function that accepts a single params object is very flexible in terms of future compatibility.
There was a problem hiding this comment.
Mostly to standardize how the editors were calling this. For example, navigation-editor was doing:
settings.__experimentalFetchLinkSuggestions = (
searchText,
searchOptions
) => fetchLinkSuggestions( searchText, searchOptions, settings );
I don't have strong feelings on this one. I can try preferring the simple signature.
| * | ||
| */ | ||
| const createFetchLinkSuggestions = ( { | ||
| disablePostFormats = false, |
There was a problem hiding this comment.
Minor, but I think it's easier to understand options like this when they're framed positively instead of negatively because then you are less likely to end up in situations where there's a double negative. So enablePostFormats instead of disablePostFormats.
There was a problem hiding this comment.
This one is an editor setting, happy to update this, but it might be better in a follow up PR to avoid other regressions.
| type = undefined, | ||
| subtype = undefined, | ||
| page = undefined, | ||
| perPageArg = undefined, |
There was a problem hiding this comment.
Looking at the previous fetchLinkSuggestions implementations, it looks like this should be perPage not perPageArg.
| perPageArg = undefined, | |
| perPage: perPageArg = undefined, |
There was a problem hiding this comment.
Good catch, it was used to avoid to avoid redeclaring a const, but I shouldn't have added it to the function signiture. https://github.com/WordPress/gutenberg/pull/29993/files#diff-278a4a275170c4215a2f4e9d5c0c6d831f9328258220a76cda04b8d9f39f01a5L47
| ); | ||
| } | ||
|
|
||
| if ( ! disablePostFormats && ( ! type || type === 'post-format' ) ) { |
There was a problem hiding this comment.
It might be worth taking this opportunity to write some unit tests for this function since it has a bunch of conditional logic that is often error prone and apiFetch is easy to mock.
… on post, site and navigation editor
…n the same way, remove usage of lodash partialRight
…earch support for edit-widgets
gwwar
force-pushed
the
fix/navigation-link-search
branch
from
3f37473
to
20209c7
Compare
There was a problem hiding this comment.
Great catches @noisysocks, esp with the widgets editor!. It had never been wired up before so I didn't think we used navigation links there. 20209c7 should handle most of the noted feedback.
I'll go back and add some unit tests after lunch 🥪
| * @return { WPFetchLinkSuggestions } Function that fetches link suggestions | ||
| * | ||
| */ | ||
| const createFetchLinkSuggestions = ( { |
There was a problem hiding this comment.
Mostly to standardize how the editors were calling this. For example, navigation-editor was doing:
settings.__experimentalFetchLinkSuggestions = (
searchText,
searchOptions
) => fetchLinkSuggestions( searchText, searchOptions, settings );
I don't have strong feelings on this one. I can try preferring the simple signature.
| * | ||
| */ | ||
| const createFetchLinkSuggestions = ( { | ||
| disablePostFormats = false, |
There was a problem hiding this comment.
This one is an editor setting, happy to update this, but it might be better in a follow up PR to avoid other regressions.
| type = undefined, | ||
| subtype = undefined, | ||
| page = undefined, | ||
| perPageArg = undefined, |
There was a problem hiding this comment.
Good catch, it was used to avoid to avoid redeclaring a const, but I shouldn't have added it to the function signiture. https://github.com/WordPress/gutenberg/pull/29993/files#diff-278a4a275170c4215a2f4e9d5c0c6d831f9328258220a76cda04b8d9f39f01a5L47
gwwar
changed the title
|
✅ Added unit tests. They did catch an error with not specifying an optional argument ✨ |
|
Thanks for the reviews! I'm going to merge this one, but it shouldn't be an issue to move this elsewhere in a follow up if we find a better package for this. |
gziolo
added
[Type] Experimental
Fixes #29396 where only post links were returned in the navigation block in site-editor.
We have multiple implementations of
fetchLinkSuggestionsfor post-editor, site-editor, and navigation-editor. The function was copy pasted, and functionality drifted over time. Many didn't realize that the site-editor implementation existed, as it doesn't appear to have ever supported links other than posts.This PR moves a default implementation into the block-editor package to avoid any drift in behavior. Please let me know if folks have preferences for moving the function to another package.This PR moves a default implementation into a new package called@wordpress/wordpress-block-editor-configto avoid any drift in behavior. This package is used to house common settings for WordPress specific block-editors.This PR moves a default implementation into the core-data package to avoid any drift in behavior
Testing Instructions
Site Editor
To enable the site editor:
/navigationWe can insert links of varying types:
site-editor.mp4
Drafts may be created:
draft.mp4
Post Editor
We can create various links in the post editor:
post-editor.mp4
Navigation Editor
/wp-admin/admin.php?page=gutenberg-experimentsEnable Navigation screenand save/wp-admin/admin.php?page=gutenberg-navigationWe can create various links in the navigation editor
navigation-editor.mp4
Widgets Editor
/wp-admin/admin.php?page=gutenberg-experimentsEnable Widgets screen in Customizerand save/wp-admin/themes.php?page=gutenberg-widgetsWe can create various links in the widgets editor