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

Block Directory: Fix "no result" ui flash #22783

Merged
merged 12 commits into from
Jun 22, 2020

Conversation

StevenDufresne
Copy link
Contributor

Description

When searching for block directory blocks, we flash "No blocks found in your library" and then
switch it out with a result from the block directory.

Ideally, the user should never see "No blocks found in your library" while there is still a request pending.

How has this been tested?

Typing slowly (to avoid debouncing), one letter at a time,

  • Search for "github" (or any block).
  • Notice the "no results" flash before it shows the results.

Watch the gif below ⬇️ .

Screenshots

The issue

Types of changes

The problem arises because we are queuing API requests while the user is typing. We try to reduce the number of request by debouncing but it's still common to queue up multiple requests.

Before:

  • When we request, we set the property isRequestingDownloadableBlocks to true.
  • When the request finishes, we set the property isRequestingDownloadableBlocks to false.

If the user has 2 requests pending at once, isRequestingDownloadableBlocks is set to false once the first request resolves even though the latest request is still in transit.

After

  • Use a counter named pendingSearchRequests instead of a boolean value to track requests.
  • Update the selector to return true if there is > 0 requests pending.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@StevenDufresne StevenDufresne added [Type] Bug An existing feature does not function as intended [Feature] Block Directory Related to the Block Directory, a repository of block plugins labels Jun 1, 2020
@StevenDufresne StevenDufresne changed the title Fix/block directory no result flash Block Directory: Fix no result ui flash Jun 1, 2020
@StevenDufresne StevenDufresne changed the title Block Directory: Fix no result ui flash Block Directory: Fix "no result" ui flash Jun 1, 2020
@github-actions
Copy link

github-actions bot commented Jun 1, 2020

Size Change: +1 B

Total Size: 1.12 MB

Filename Size Change
build/block-directory/index.js 7.26 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 937 B 0 B
build/block-directory/style.css 937 B 0 B
build/block-editor/index.js 107 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.78 kB 0 B
build/block-library/editor.css 7.78 kB 0 B
build/block-library/index.js 129 kB 0 B
build/block-library/style-rtl.css 8.02 kB 0 B
build/block-library/style.css 8.02 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 196 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.6 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.26 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.5 kB 0 B
build/edit-post/style.css 5.5 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.33 kB 0 B
build/edit-widgets/style-rtl.css 2.43 kB 0 B
build/edit-widgets/style.css 2.43 kB 0 B
build/editor/editor-styles-rtl.css 468 B 0 B
build/editor/editor-styles.css 469 B 0 B
build/editor/index.js 44.7 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 450 B 0 B
build/list-reusable-blocks/style.css 451 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 663 B 0 B
build/nux/style.css 660 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ryelle
Copy link
Contributor

ryelle commented Jun 1, 2020

The counter approach feels unintuitive to me, what about restructuring downloadableBlocks to track each specific request? Like this:

downloadableBlocks.{filterValue}.results = [...]
downloadableBlocks.{filterValue}.isRequesting = true|false

Then isRequestingDownloadableBlocks selector would use filterValue to check if that specific request is done yet.

@StevenDufresne
Copy link
Contributor Author

StevenDufresne commented Jun 2, 2020

The counter approach feels unintuitive to me, what about restructuring downloadableBlocks to track each specific request? Like this:

downloadableBlocks.{filterValue}.results = [...]
downloadableBlocks.{filterValue}.isRequesting = true|false

Then isRequestingDownloadableBlocks selector would use filterValue to check if that specific request is done yet.

That's where my head went first as well. I tracked back under the following unordered assumptions:

  • Do we need to know about the requests?
    • Maybe if we want a request caching mechanism, but in reality, since we request on key{down/up} event, we would collect a lot of useless result sets.
    • There is a low likely that the same search is ran twice
    • We could also run into some race conditions where the component checks with an outdated filterValue.
  • We really only want to know 1 thing: Is there a request pending?
    • A counter seemed the easiest check since a boolean doesn't account for multiple pending requests.
    • Storing more data to make the same determination is probably over engineering.

Thoughts?

@ryelle
Copy link
Contributor

ryelle commented Jun 2, 2020

We're already storing the requests for each response per filterValue (right now it's downloadableBlocks.results.{filterValue} instead), so we're already saving those extra responses, this would just bring up parity to the results list. And IMO we should save those, to ensure we're showing the correct results for the given filterValue (re: repeated searches, i think it's less likely, but still a use case— typos, searching for a different product block, etc).

If we trigger 3 requests when typing, maybe for prod, produc, then land on product, we don't actually need to know when prod & produc finish, we only want the current filterValue. So it's not that we need to know if any requests are pending, we just need to know about the current request.

@StevenDufresne
Copy link
Contributor Author

We're already storing the requests for each response per filterValue (right now it's downloadableBlocks.results.{filterValue} instead), so we're already saving those extra responses, this would just bring up parity to the results list.

You're right, it didn't register that we were already caching them and how that ties into the selector/resolver.

So it's not that we need to know if any requests are pending, we just need to know about the current request.

That's true.

I'm not 💯 convinced that we need to keep the result set in memory. I think it could get a bit messy when we start passing more context to the block directory to provide better search results in the future (theres no plan, just an idea)... But I don't mind tracking the progress on the specific request for now. I'll update.

@StevenDufresne
Copy link
Contributor Author

@ryelle Mind taking another pass through this one? Thanks :)

Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Working great in browser, just some comments about the tests.

Copy link
Contributor Author

@StevenDufresne StevenDufresne left a comment

Choose a reason for hiding this comment

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

How is this test different from the others?

Weird, totally unnecessary. Thanks. 😕

@StevenDufresne StevenDufresne force-pushed the fix/block-directory-no-result-flash branch from 885b1a3 to afc0377 Compare June 10, 2020 01:52
@StevenDufresne
Copy link
Contributor Author

@ryelle Anything left here that you think should be addressed?

Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Looks good, once the conflict is resolved this can be merged 👍

@StevenDufresne StevenDufresne force-pushed the fix/block-directory-no-result-flash branch 4 times, most recently from dbe82d0 to aede0f9 Compare June 19, 2020 05:59
@StevenDufresne StevenDufresne force-pushed the fix/block-directory-no-result-flash branch from aede0f9 to df0d8ec Compare June 21, 2020 23:51
@StevenDufresne StevenDufresne merged commit a3e1255 into master Jun 22, 2020
@StevenDufresne StevenDufresne deleted the fix/block-directory-no-result-flash branch June 22, 2020 01:36
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 22, 2020
This was referenced Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Directory Related to the Block Directory, a repository of block plugins [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants