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

[RNMobile] Add inserter block search #29169

Merged
merged 19 commits into from
Mar 18, 2021
Merged

[RNMobile] Add inserter block search #29169

merged 19 commits into from
Mar 18, 2021

Conversation

jhnstn
Copy link
Contributor

@jhnstn jhnstn commented Feb 19, 2021

Description

Adds a block search form to the existing search menu. The available blocks are filtered according to the search query.
The search form is only rendered when a minimal number of blocks are available in the menu (the current min is set to 2)

The new feature is behind the __DEV__ flag in this change set.

Part of wordpress-mobile/gutenberg-mobile#2570

How has this been tested?

Testing search flow

  • Open the mobile block inserter

  • Press "Search blocks"

  • Try searching for a block with the soft keyboard

    • The inserter menu should filter the blocks per the search string
  • Try pressing on a block after searching

    • The chosen block should be inserted in the expected position
    • The inserter menu should close
  • Press "Search blocks"

    • The prior search input should be cleared
  • Try searching with a single character i.e, i

    • The blocks in the menu should be sorted to best match the search
    • The menu should be scrollable

Testing canceling search flows

  • Open the mobile block inserter

  • Press "Search blocks"

  • Press the arrow on the left side of the input

    • The inserter menu should return to the original inserter menu
  • Press "Search blocks" again

  • Start searching for a block

  • Press the x on the right side of the input

    • The input field should clear and the inserter menu should show all available blocks
  • Start searching for a block again

  • Press the arrow on the left side of the input

  • Press "Search blocks" again

    • The input field should reset to the placeholder text

Testing with few available blocks

  • Add a Buttons block
  • With the Buttons block selected, open the block inserter
    • The search form should not be visible

Screenshots

inseter-block-search mp4

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Feb 19, 2021
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @jhnstn! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

Base automatically changed from master to trunk March 1, 2021 15:45
@jhnstn jhnstn marked this pull request as ready for review March 5, 2021 23:39
@jhnstn jhnstn changed the title [RNMobile] WIP Add inserter block search [RNMobile] Add inserter block search Mar 10, 2021
Copy link
Contributor

@illusaen illusaen left a comment

Choose a reason for hiding this comment

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

So I know you didn't work on the actual filtering part on this PR.... but the search filter seems a little wonky? See below GIF for details.

Wonky search

search

icon={ arrowLeft }
onClick={ () => {
inputRef.current.blur();
onChange( '' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiousity, why default to empty string instead of undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I was thinking the same when I looked back at this PR 😆 . After reviewing the changes as they are now I can't find an explicit reason. I believe as some point while I was drafting the changes I was treating undefined differently. That's not the case now.

I might update the onChange function definition to use '' as the default value in a later change. That way the value remains a String type but can be called with undefined

Copy link
Contributor

@illusaen illusaen left a comment

Choose a reason for hiding this comment

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

Weird search is being handled in other PR. This one works well.

@jhnstn
Copy link
Contributor Author

jhnstn commented Mar 11, 2021

So I know you didn't work on the actual filtering part on this PR.... but the search filter seems a little wonky? See below GIF for details.

Yes, I noticed that too. The fix is in a lower level component which I didn't want to include in this new work. The trick is the fix is easier to verify with this change set. Here is the fix PR #29274

Comment on lines +148 to +152
let itemsToDisplay = items.filter(
( { name } ) => name !== 'core/block'
);

itemsToDisplay = searchItems( itemsToDisplay, filterValue );
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing, but I wonder if instead of using a let here we could used two const variables with names that showed how they were different.

@mchowning
Copy link
Contributor

Thanks for the PR @jhnstn and the review @illusaen ! 🙇

@mchowning mchowning merged commit 0e4fcfd into WordPress:trunk Mar 18, 2021
@github-actions github-actions bot added this to the Gutenberg 10.3 milestone Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants