Introducing EuiSearchBar #379
Conversation
There was a problem hiding this comment.
Gave you a small design PR to merge into this for some small stuff I noticed. https://github.com/uboness/eui/pull/7
Functionally works as I'd expect. You should document the props to this (as a tab) in the example page you have set up. I know you do it your text, but nice to have it in there as well for the auto-gen stuff.
++ on it |
|
@snide another review? |
|
Something weird happen with the bundle? Says that it changed 206k lines? Functionally it looks correct and matches the styling we had set up. Couple comments on the docs.
Other docs changes are minor that I can do in a follow up. |
|
I'll have to look at the size... Not sure what it is. I can add the missing filters to the examples The example does actually use the basic - If you ran the latest example you should see it to the left of the ES query code snippet |
|
Ha. That's what I get for looking at code on a sunday night. Forgot to pull. This looks fantastic. Love that you expose the search query in the docs. |
|
updated docs example with additional filters |
|
fixed the size (I accidentally changed the docs bundle... which caused the 200K lines diff) |
|
@nreese or @pickypg or @tsullivan have time to review this one since you all have been waiting on it? Functionally it works the way id expect, but I don't feel comfortable reviewing the code. From my side its good to go minus some small cleanup (mostly docs language and nit-picky styles) that I can do in a follow up. |
|
@snide @AlonaNadler fyi, added or clause support (more or less aligned that with kql - |
| ); | ||
| } | ||
|
|
||
| if (href) { |
There was a problem hiding this comment.
This code is very similar to the EuiLink code (although EuiLink becomes a button based on the presence of onClick). Why replicate it here rather than
<EuiLink
className={classes}
disabled={isDisabled}
{...rest}
>
<span className="euiFilterButton__content">
{buttonIcon}
<span className="euiFilterButton__textShift" data-text={children}>{children}</span>
</span>
</EuiLink>Is it that the euiLink CSS interferes?
There was a problem hiding this comment.
This one is me. It is similar, but stylistically different on the CSS side. Essentially I'm future proofing its functionality for some needs we always end up having when we force clickable "buttons" into sometimes being hrefs, sometimes being buttons...etc.
There was a problem hiding this comment.
We should probably create a service function to follow this pattern then to avoid inconsistently rewriting it again later. Not asking for that to be done here though.
There was a problem hiding this comment.
Yeah, I think that's a good idea. It's all over our button components. I'll set up a separate issue.
| name: PropTypes.string, | ||
| id: PropTypes.string, | ||
| placeholder: PropTypes.string, | ||
| value: PropTypes.string, | ||
| isInvalid: PropTypes.bool, | ||
| fullWidth: PropTypes.bool, | ||
| isLoading: PropTypes.bool, | ||
| inputRef: PropTypes.func, | ||
| onSearch: PropTypes.func, |
There was a problem hiding this comment.
Any reason to not make this a .isRequired property? Example pages could supply () => {} to ignore it.
There was a problem hiding this comment.
I don't think this is a consumer friendly API that you need to force them to provide empty callback, it'll will account to a hacky code
There was a problem hiding this comment.
What real usage wouldn't want to specify the callback? As it is, you're stuck having to check that it is defined on [current] line 49.
There was a problem hiding this comment.
I mind less the extra check... but to your question, the user can still just listen to change events rather than search events. in other words, I see onSearch as a convenient callback but not necessarily as the mandatory to use one (at the end of the day, it is a low level component and the less restrictions you put, the better IMO)
| name: PropTypes.string, | ||
| id: PropTypes.string, | ||
| placeholder: PropTypes.string, | ||
| value: PropTypes.string, | ||
| isInvalid: PropTypes.bool, | ||
| fullWidth: PropTypes.bool, | ||
| isLoading: PropTypes.bool, | ||
| inputRef: PropTypes.func, | ||
| onSearch: PropTypes.func, | ||
| incremental: PropTypes.bool |
There was a problem hiding this comment.
So, does incremental basically mean typeahead support?
There was a problem hiding this comment.
no... typeahead tries to predict what the user input is and offers to complete the input... this is not what incremental is... incremental means that "as you type" search... basically will continuously execute the search as you type. name taken from not-yet-standard https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#attr-incremental
There was a problem hiding this comment.
Ah, that's why I didn't recognize it. Perhaps we can link to that somewhere?
There was a problem hiding this comment.
I prefer not linking it because it may give the wrong impression that we're depending on it (and it's not a standard attribute yet)... I can add more jsdocs around it though...
| }; | ||
|
|
||
| EuiFieldSearch.defaultProps = { | ||
| const defaultProps = { | ||
| value: undefined, |
There was a problem hiding this comment.
Pre-existing: Isn't that implied?
There was a problem hiding this comment.
it is... copy it as is was, but yea... will remove
There was a problem hiding this comment.
it is... copy it as is was, but yea... will remove
| export class EuiFieldSearch extends React.Component { | ||
|
|
||
| static propTypes = propTypes; | ||
| static defaultProps = defaultProps; |
There was a problem hiding this comment.
Any reason to not just define these in the class directly? The indirection seems unnecessary (although I get that it kind of already existed).
There was a problem hiding this comment.
I find it cleaner when it's outside... also sometimes you need to put the prop types outside and export them for reusability.. so I prefer always putting them outside for consistency.. but yea... there aren't really best practices that we defined here or anything like that
| } | ||
|
|
||
| componentWillUpdate(nextProps) { | ||
| this.inputElement.value = nextProps.query; |
There was a problem hiding this comment.
Why not just pass in query as value into the EuiFieldSearch below and make this a stateless function? Then I think the inputRef part can be dropped too?
There was a problem hiding this comment.
you can't do that if you want to control the input from both the inside and the outside..
There was a problem hiding this comment.
But why try to control the input from inside and out? That breaks encapsulation and beyond this line it's unused.
There was a problem hiding this comment.
what encapsulation? sometimes you simple need access to underlying core elements (it's not uncommon). In any case, it's required here because the query is updated both ways (from the outside by the search filters, and from the inside by the search input)
|
|
||
| render() { | ||
| return ( | ||
| <div> |
There was a problem hiding this comment.
Does this need to be wrapped in a div? If not I wonder if we should get into the habit of returning Fragments instead so that the idea propagates throughout usage of EUI.
There was a problem hiding this comment.
I'm not sure what's your suggestion... that some components will return fragments and others will return single elements? I'm not sure what's the problem here really, what's wrong with having the component be represented as a single element. I guess my question is, what problem are you suggesting to solve?
There was a problem hiding this comment.
For the top-level <div>:
<div>
...
</div>
replaced with
<Fragment>
...
</Fragment>
That way the loaded HTML not cluttered with unnecessary <div> components.
There was a problem hiding this comment.
oh... I understand now... my next question would be... why?
There was a problem hiding this comment.
Same reason as React says to use it: to avoid cluttering with unnecessary <div> tags, but also as a means to avoid impacting how things look based on the <div> being there. It's more about establishing the pattern than it's a problem here.
There was a problem hiding this comment.
I think @pickypg is right, but it's also not critical. Generally I think we should move to use <Fragment> wherever possible, for the reasons he stated. However, I know Enzyme has some issues with testing components using Fragments, so if we run into that issue we'll just have to stick with <div>. All in all, I'd say it's up to you @uboness.... using <Fragment> would be a best practice IMO, but it's not the end of the world if we keep the <div>.
There was a problem hiding this comment.
Ah, I didn't realize this is an example in the docs. So my comment about testing problems doesn't apply here. I'm slightly more in favor of using <Fragment> now since it really wouldn't hurt. Still, it's your call Uri.
There was a problem hiding this comment.
I think it ultimately doesn't matter. We only very rarely do css selectors that go against things like > * which is the only time where something like this could cause trouble in a display.
I actually hadn't used fragment much before, i think some of it is fairly new. I think you can even use </> which is even more confusing. FWIW, an extra naked div usually isn't cause for concern most times, but <fragment> seems the preferred way to do this stuff.
There was a problem hiding this comment.
yea... it's an example.
my take: it really doesn't matter... ane extra div doesn't kill anyone... but sure.. it also doesn't contribute much.
I updated to Fragment, but I honestly don't think it's of much value - if anything, without a clear guideline here, it causes more confusion than anything else...
| this.state = { hasFocus: false }; | ||
| } | ||
|
|
||
| focus() { |
There was a problem hiding this comment.
Nitpick: some of these are defined as functions directly versus others, like onFocus below are assigned functions.
There was a problem hiding this comment.
there's actually a good reason to have different forms of functions in a component... that said, in this case, you're right... focus should be a member function as it should be exposed when the components is refed
|
|
||
| export const _termValuesToQuery = (values, options) => { | ||
| const body = { | ||
| query: values.join(' ') |
There was a problem hiding this comment.
Does this need to be trimmed for the next check?
| const terms = collectTerms(ast); | ||
| const fields = collectFields(ast); | ||
|
|
||
| const must = []; |
There was a problem hiding this comment.
The repetitive nature of this between must and mustNot made this harder to read, but it looks good.
There was a problem hiding this comment.
I considered that... but sometimes it's better to keep things explicit and more verbose yet readable rather than trying to create reusable function that hide the logic
|
@uboness played with the PR a bit,
Overall Looks helpful! |
sorry... not following
the official language to do that is
valid... I was thinking that design wise we could perhaps create groups of filters... but I'll leave tot @snide to handle design. Also, as a side note, I don't expect the toggle group (e.g. open/closed duo) should be used next to other filters... I modeled it after the toolbar in github issues:
as you can see here ^ they really separated these from the other filters and this separation makes the different nature of the control more intuitive/digestible I guess. |
A powerful search bar for common use cases that require more than just prefix and terms lookups. This one also supports field clauses (e.g. `tag:bug`) and is clauses (e.g. `is:open`). The bar has a search box where the user can enter the query text and can also hold filters. Filter are essentially UI controls that help to manipulate/build the query - they mainly help those the don't remember the query syntax.
- with `onParse` it's now possible to get continuous feedback about the query text parsing. The example demo shows how it can be used to provide a feedback about the validity of the query. In the future we might be able to use it for other things as well (e.g. auto-completion) - Enhanced the example demo quite a bit to show off a few features: ES query translation, JS array execution, incremental search and query validation.
- added `field_value_toggle_group` filter - added `field_value_toggle` filter Also: - cleaned up `FieldValueToggleGroupFilter` a bit
- Updated `field_value_selection` filter to support both `and` and `or` clauses. Now when configured to be `multiSelect` - the `multiSelect` attribute can be set to `or` or `and` (setting to `true` is as setting it to `and`).
- Syntax: the syntax for or field clauses is: `field:(value1 or value2)`
- Also added support for phrases: `"this is a term phrase" -text:"and this is a field phrase" hamlet:("to be" or "not to be")`
We have a loading mechanism coming in a separate PR that more deals with the results of the search (since it will be different for each type). It will look similar to this (but likely account for if you at the bottom of a table)
Yep. This is mostly just example documentation to explain usage to developers. I don't think putting this many filters that all do the same thing would be something we'd do in practice. |
A powerful search bar for common use cases that require more than just prefix and terms lookups. This one also supports field clauses (e.g.
tag:bug) and is clauses (e.g.is:open).The bar has a search box where the user can enter the query text and can also hold filters. Filter are essentially UI controls that help to manipulate/build the query - they mainly help those the don't remember the query syntax.