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

Filter picker search free input #38907

Merged
merged 7 commits into from
Feb 22, 2024
Merged

Conversation

ranquild
Copy link
Contributor

@ranquild ranquild commented Feb 19, 2024

Product doc https://www.notion.so/metabase/Free-up-text-input-performance-from-loading-values-in-filters-f70287f73d014794abb60653c6d28e39
Fixes #37906

Previously it wasn't possible to select a value not from the list for "searchable" columns. It becomes a problem when it takes a lot of time to load search results as they are loaded from the customer DB directly. In this case the user is not able to create a filter at all.

Now we allow entering any value, even without waiting for search results (but no duplicates). This is based on creatable property of the mantine's MultiSelect. I've decided to go with New {input} label but this is to be verified with Product.

How to verify:

  • New -> Question -> People
  • Filter by Email
  • Trying adding filters

Before

With search results:
Screenshot 2024-02-19 at 14 46 04

No search results:
Screenshot 2024-02-19 at 14 46 10

After

With search results:
Screenshot 2024-02-19 at 14 39 28

No search results:
Screenshot 2024-02-19 at 14 39 09

@ranquild ranquild added no-backport Do not backport this PR to any branch backport Automatically create PR on current release branch on merge and removed no-backport Do not backport this PR to any branch labels Feb 19, 2024
Copy link

replay-io bot commented Feb 19, 2024

Status Complete ↗︎
Commit 20b23f1
Results
⚠️ 3 Flaky
2315 Passed

@ranquild ranquild requested review from mazameli and a team February 19, 2024 12:57
@ranquild ranquild added this to the 0.49 milestone Feb 19, 2024
});

userEvent.type(screen.getByPlaceholderText("Search by Email"), "a@b.com");
userEvent.hover(screen.getByText("New a@b.com"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason mantine doesn't work without hovering the item first :(

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be helpful if this comment lived in the code

Copy link
Contributor

@kamilmielnik kamilmielnik left a comment

Choose a reason for hiding this comment

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

LGTM 👍


userEvent.type(screen.getByLabelText("Filter value"), "a@b.com");
expect(screen.queryByText("New a@b.com")).not.toBeInTheDocument();
expect(onChange).not.toHaveBeenCalled();
Copy link
Contributor

@kamilmielnik kamilmielnik Feb 20, 2024

Choose a reason for hiding this comment

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

Let's try to hit Enter before the assertions

});

userEvent.type(screen.getByPlaceholderText("Search by Email"), "a@b.com");
userEvent.hover(screen.getByText("New a@b.com"));
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be helpful if this comment lived in the code

@@ -63,12 +67,15 @@ export function SearchValuePicker({
value={selectedValues}
searchValue={searchValue}
placeholder={placeholder}
nothingFound={isSearched ? nothingFound : null}
shouldCreate={shouldCreate}
getCreateLabel={searchValue => t`New ${searchValue}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @cdeweyx @mazameli

  1. "New" suggests that something will be created. I'd skip the word and just use the value itself.
  2. It's not ideal when this value is last in the options list because it can't be seen when there are other results:
    image

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Ideally the "new" value should not be displayed when an exact match is found:

image

Copy link

@cdeweyx cdeweyx left a comment

Choose a reason for hiding this comment

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

Looking good

@ranquild ranquild merged commit e813357 into master Feb 22, 2024
107 checks passed
@ranquild ranquild deleted the filter-picker-search-free-input branch February 22, 2024 18:01
github-actions bot pushed a commit that referenced this pull request Feb 22, 2024
metabase-bot bot added a commit that referenced this pull request Feb 22, 2024
Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Is" filter in the header of the columns in a table context should be linked and allow free input
3 participants