-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiInMemoryTable] Allow consumers to use non-EQL plain text search with special characters #7175
Changes from 6 commits
463a854
ff7f6ed
65e5205
bf1235e
74f96cc
4d8fd29
17d31b5
feb7a91
40d2ea3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,11 +23,15 @@ import { PropertySort } from '../../services'; | |
import { Pagination as PaginationBarType } from './pagination_bar'; | ||
import { isString } from '../../services/predicate'; | ||
import { Comparators, Direction } from '../../services/sort'; | ||
import { EuiSearchBar, Query } from '../search_bar'; | ||
import { | ||
EuiSearchBar, | ||
EuiSearchBarProps, | ||
Query, | ||
SchemaType, | ||
} from '../search_bar/search_bar'; | ||
import { EuiSearchBox } from '../search_bar/search_box'; | ||
import { EuiSpacer } from '../spacer'; | ||
import { CommonProps } from '../common'; | ||
import { EuiSearchBarProps } from '../search_bar/search_bar'; | ||
import { SchemaType } from '../search_bar/search_box'; | ||
import { | ||
EuiTablePaginationProps, | ||
euiTablePaginationDefaults, | ||
|
@@ -76,6 +80,15 @@ type InMemoryTableProps<T> = Omit< | |
* Configures #Search. | ||
*/ | ||
search?: Search; | ||
/** | ||
* If passed as true, search ignores all filters and EQL syntax, and anything | ||
* typed into the table search bar is treated as plain text. | ||
* | ||
* This functionality allows users to search for strings with special characters | ||
* such as quotes, parentheses, and colons, which are normally otherwise | ||
* reserved for EQL syntax. | ||
*/ | ||
searchPlainText?: boolean; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% convinced this PR is the right way to go, so I'm opening it to get earlier thoughts from the rest of the team.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just thinking out loud, what about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I love it! I'll go with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, leaving some extra thoughts just for more context, in case future devs come by this PR and are like "why did they do things this way":
When I mentioned this above, I was referring to the fact that this "plain text" search is still essentially using EQL under the hood, primarily to execute over every However, in the end, I think this is a simpler approach to go with because while skipping EQL entirely might be more performant, this at least isn't less performant than current behavior, and additionally has much less dev overhead/testing required (we'd have to write a brand new utility to execute/iterate over every single item otherwise). In the future, something might change to necessitate a bigger break between EQL and non EQL search - but in the interim, I think this is an okay compromise between performance and developer time spent. |
||
/** | ||
* Configures #Pagination | ||
*/ | ||
|
@@ -521,9 +534,34 @@ export class EuiInMemoryTable<T> extends Component< | |
})); | ||
}; | ||
|
||
// Alternative to onQueryChange - allows consumers to specify they want the | ||
// search bar to ignore EQL syntax and only use the searchbar for plain text | ||
onPlainTextSearch = (searchValue: string) => { | ||
const escapedQueryText = searchValue.replaceAll('"', '\\"'); | ||
const finalQuery = `"${escapedQueryText}"`; | ||
cee-chen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.setState({ | ||
query: EuiSearchBar.Query.parse(finalQuery), | ||
}); | ||
}; | ||
|
||
renderSearchBar() { | ||
const { search } = this.props; | ||
if (search) { | ||
const { search, searchPlainText } = this.props; | ||
if (!search) return; | ||
|
||
let searchBar: ReactNode; | ||
|
||
if (searchPlainText) { | ||
const _searchBoxProps = (search as EuiSearchBarProps)?.box || {}; // Work around | boolean type | ||
const { schema, ...searchBoxProps } = _searchBoxProps; // Destructure `schema` so it doesn't get rendered to DOM | ||
cee-chen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
searchBar = ( | ||
<EuiSearchBox | ||
query="" // Unused, passed to satisfy Typescript | ||
{...searchBoxProps} | ||
onSearch={this.onPlainTextSearch} | ||
/> | ||
); | ||
} else { | ||
let searchBarProps: Omit<EuiSearchBarProps, 'onChange'> = {}; | ||
|
||
if (isEuiSearchBarProps(search)) { | ||
|
@@ -538,13 +576,17 @@ export class EuiInMemoryTable<T> extends Component< | |
} | ||
} | ||
|
||
return ( | ||
<> | ||
<EuiSearchBar onChange={this.onQueryChange} {...searchBarProps} /> | ||
<EuiSpacer size="l" /> | ||
</> | ||
searchBar = ( | ||
<EuiSearchBar onChange={this.onQueryChange} {...searchBarProps} /> | ||
); | ||
} | ||
|
||
return ( | ||
<> | ||
{searchBar} | ||
<EuiSpacer size="l" /> | ||
</> | ||
); | ||
} | ||
|
||
resolveSearchSchema(): SchemaType { | ||
|
@@ -653,6 +695,7 @@ export class EuiInMemoryTable<T> extends Component< | |
tableLayout, | ||
items: _unuseditems, | ||
search, | ||
searchPlainText, | ||
onTableChange, | ||
executeQueryOptions, | ||
allowNeutralSort, | ||
|
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.
Upgrade needed for the new
faker.string.symbol
API I'm using to generate random special characters. Probably safe to do now that the faker controversy is behind us 🙈