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

Remove @wordpress/api-fetch usage from the block editor module #14527

Merged
merged 2 commits into from
Mar 21, 2019

Conversation

youknowriad
Copy link
Contributor

Related #14043

The idea is that the block editor module is independent from WordPress or Rest APIs, it should be usable in any context.

In this PR, I'm removing the apiFetch usage by providing a fetchLinkSuggestions setting to the editor settings. So by default URL inputs in any block editor will work properly but without any suggestions, if you want to enable suggestions, you provide a function returning a promise with the suggestions.

Testing instructions

  • Check that the adding links work properly
  • Check that you have post/pages suggestions when typing in the Url Input.

@youknowriad youknowriad self-assigned this Mar 20, 2019
@youknowriad youknowriad added the [Type] Code Quality Issues or PRs that relate to code quality label Mar 20, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Everything works properly with all those changes applied.

I like that fetchLinkSuggestions is now passed as a prop to the component. It's also acceptable to put it inside block editor setting for the time being as an experimental feature __experimentalFetchLinkSuggestions.

At the same time, I want to make sure we have a follow-up issue opened which will try to find a long-term solution for extensibility in this area. We have 3 experimental settings which aren't really setting value:

  • __experimentalFetchLinkSuggestions
  • __experimentalMediaUpload
  • __experimentalReusableBlocks

All 3 items are closer to data actions which could be implemented internally with controls maybe. Another way of approaching this complex use case would be through context API (maybe with React hooks?).

@youknowriad
Copy link
Contributor Author

I feel having callbacks in settings is fine. the two first ones mentionned here are experimental because I'd prefer to have some time to validate the proposal but they still feel like block editor settings for me.

The last one is ideally removed but I'm not sure at this point if it's possible or not.

@aduth
Copy link
Member

aduth commented Mar 21, 2019

Similarly, without a deep review, this looked good on the surface. For future exploration, I'd wondered if it could be something along the lines of what was explored previously in #9210 in allowing custom resolver behavior to be implemented by the extending context. I guess the idea of "the editor component" implementing a resolver is non-conventional, but on the consuming end, it seems nice the idea of a fetchLinkSuggestions selector which is stubbed by default as an "abstract" selector.

@youknowriad youknowriad merged commit 98a19b6 into master Mar 21, 2019
@youknowriad youknowriad deleted the update/remove-api-fetch-block-editor branch March 21, 2019 18:08
@gziolo
Copy link
Member

gziolo commented Mar 22, 2019

I guess the idea of "the editor component" implementing a resolver is non-conventional, but on the consuming end, it seems nice the idea of a fetchLinkSuggestions selector which is stubbed by default as an "abstract" selector.

Thinking a bit more about this idea, I'm coming to the conclusion that this might be less flexible than using context API or the existing BlockEditorProvider component. In particular, this could be super handy in the case of nested editors. Reusable blocks are an immediate example that comes to my mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants