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

Editor: Trigger reusable blocks autocomplete when options generated #14915

Merged
merged 1 commit into from
Apr 17, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 10, 2019

Fixes #14766

This pull request changes the behavior of the default block autocompleter to avoid triggering a fetch of reusable blocks until the time at which the user begins entering a search query which would prompt the block autocompleter to generate its own options set. Prior to this change, a fetch would occur immediately upon selecting a paragraph block. While this had the benefit of preloading data for improved likelihood could be reflected in the search query, it had the downsides of (a) not guaranteeing this likelihood, (b) incurring the overhead of a network request, and (c) causing changes in state. The last of these manifests itself in the bug report described in #14766.

For transparency's sake, I should be clear that the implementation here does not solve the underlying issue that receiving reusable blocks into editor state has the dual effect of creating an undo level and marking the post as having unsaved changes. This is tracked more thoroughly at #14910, with a separate improvement planned. Thus, it is still expected that unexpected undo history will accumulate in response to reusable blocks fetching (e.g. clicking the Inserter when on a new post will add an Undo level). Technically speaking, the full fix considered for #14910 at #14910 (comment) would resolve both #14910 and #14766, at which point this pull request could be considered an optional enhancement / change in behavior to defer fetching of reusable blocks.

Separately, I anticipate that we should explore further refactoring of autocompleters to avoid "fetches" as explicit actions, and consider what solutions might exist for being able to express autocompleter options via subscribed selectors (or withSelect-bound components) and complementing resolvers.

Testing Instructions:

Repeat Steps to Reproduce from #14766, observing that no undo level is created.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Feature] History History, undo, redo, revisions, autosave. labels Apr 10, 2019
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I verified that before as soon as I focused paragraph a blocks request was immediately made, now the request is only made when the inserter or the autocomplete opens.

* been true, but relied upon the fact the user would be delayed in typing an
* autocompleter search query. Once implemented using resolvers, the status of
* this request could be subscribed to as part of a promised return value using
* the result of `hasFinishedResolution`. There is currently reliable way to
Copy link
Member

Choose a reason for hiding this comment

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

There is currently (no) reliable way to

@aduth
Copy link
Member Author

aduth commented Apr 11, 2019

Thanks for the review @jorgefilipecosta !

While this does serve as a fix for #14766, that issue was also separately fixed by #14916 . I still think this pull request can be considered an improvement to avoid the fetch until necessary, but not as urgent as it once was. I think I may wait until after the next plugin release to consider merging it. Either that, or give some more time to consider what might be necessary to support autocompleters which update options asynchronously.

@aduth aduth added [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) and removed [Type] Bug An existing feature does not function as intended [Feature] History History, undo, redo, revisions, autosave. labels Apr 11, 2019
@youknowriad
Copy link
Contributor

Going to merge this as it brings more stability to e2e tests.

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.

Merely selecting a block after load creates undo level
3 participants