-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Allow creating new items by inserting comma in MultiAutocomplete #42824
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff 80687b8...6e9b253.
|
|
setSelectedValues(newSelectedValues); | ||
setLastSelectedValues(newSelectedValues); | ||
} | ||
const text = event.clipboardData.getData("text"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Text
was breaking the test, and both Text
and text
work in my browsers.
83318af
to
c97430c
Compare
c97430c
to
b96beea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well 👍
This change obviously makes it difficult to filters/search for strings that include comma character - please make sure that we have product team approval for this.
(for example in our Sample Database there is a person with this address: 5509-5501,5500-5508 McKinley Avenue
- now it won't be possible to just paste this value into the filter input)
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.unit.spec.tsx
Outdated
Show resolved
Hide resolved
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.unit.spec.tsx
Outdated
Show resolved
Hide resolved
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx
Outdated
Show resolved
Hide resolved
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.unit.spec.tsx
Show resolved
Hide resolved
@kamilmielnik there is a follow up to add escaping for the comma character. |
@kamilmielnik re:
I've added those follow up changes here, since they were small and it doesn't make sense to merge this in the "broken" state. Please have another look. |
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/util.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works really well 👍
I think it's ready to go once we address "Case 3".
setSearchValue(""); | ||
|
||
if (values.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values
aren't used in this if/else block - is this correct?
Two more occurrences of this below.
if (values.length > 0) { | |
if (validValues.length > 0) { |
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx
Show resolved
Hide resolved
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx
Outdated
Show resolved
Hide resolved
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/util.ts
Outdated
Show resolved
Hide resolved
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx
Outdated
Show resolved
Hide resolved
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx
Show resolved
Hide resolved
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.unit.spec.tsx
Show resolved
Hide resolved
805099e
to
b783140
Compare
b783140
to
bcd93bc
Compare
|
||
if (newSearchValue !== "") { | ||
const values = parseValues(newSearchValue); | ||
if (values.length === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if values.length > 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/MultiAutocomplete.tsx
Show resolved
Hide resolved
frontend/src/metabase/ui/components/inputs/MultiAutocomplete/utils.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>
Closes #42783
Description
Allows creating new filter items by pressing the comma (
,
) key without having to blur the input.How to verify
Describe the steps to verify that the changes are working as expected.
First name
header,
, paste a list of names, add quote-delimited values like"foo, bar"
, ...Demo
Metabase.-.17.May.2024.mp4
Checklist