Euify and Reactify Query Bar Component #23704
Conversation
|
Could you "virtualize" the suggestions with something like react-virtualized? |
💔 Build Failed |
|
BTW I just tested this on my large fields test case and it resolved the issues we discussed in #14086 Let me know if you still see any issues. |
💔 Build Failed |
Bargs
added
review
Feature:Query Bar
| } | ||
| } | ||
|
|
||
| export function Suggestion(props) { |
There was a problem hiding this comment.
Could I make a suggestion? (Pardon the pun). Here's how I've seen a lot of components formatted both in EUI and in Kibana:
export function Suggestion({
selected,
suggestion,
onMouseEnter,
onClick,
innerRef,
ariaId,
}) {
const {
text,
description,
type,
} = suggestion;
const classes = classNames('typeahead-item', { active: selected });
const itemClasses = `suggestionItem suggestionItem--${type}`;
return (
<div
className={classes}
role="option"
onClick={() => onClick(suggestion)}
onMouseEnter={onMouseEnter}
ref={innerRef}
id={ariaId}
>
<div className={itemClasses}>
<div className="suggestionItem__type" type={type}>
<EuiIcon type={getEuiIconType(type)}/>
</div>
<div className="suggestionItem__text">{text}</div>
<div
className="suggestionItem__description"
// Description currently always comes from us and we escape any potential user input
// at the time we're generating the description text
// eslint-disable-next-line react/no-danger
dangerouslySetInnerHTML={{ __html: description }}
/>
</div>
</div>
);
}The notable differences are the use of destructuring and the extraction of calculated values out of the JSX itself. At first I didn't like the use of destructuring but it grew on me because it helped me identify which props I was using up-front, and it also reduced noise in the JSX. Just a suggestion, feel free to disregard.
There was a problem hiding this comment.
I'm mostly cool with destructuring and happy to follow whatever conventions folks are using for react stuff already, will make the update 👍
💔 Build Failed |
|
Hey @Bargs nice work. Want me to pass you a PR to convert the Less to Sass? Should be a pretty quick, non-breaking switch. |
|
@snide for sure! |
| // Description currently always comes from us and we escape any potential user input | ||
| // at the time we're generating the description text | ||
| // eslint-disable-next-line react/no-danger | ||
| dangerouslySetInnerHTML={{ __html: props.suggestion.description }} |
There was a problem hiding this comment.
Out of interest: Where are those descriptions actually set/coming from? Could we potentially write descriptions in JSX directly, so we wouldn't need to use dangerouslySetInnerHTML here?
There was a problem hiding this comment.
They come from the autocomplete provider types
|
Accessibility looks fine for me. I notice two other kind of strange behaviors, but both of them might actually also be EUI issues:
|
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
|
Looks like the failure is a flaky test, going to retry if github will let me right now. |
| // at the time we're generating the description text | ||
| // eslint-disable-next-line react/no-danger | ||
| // @ts-ignore | ||
| dangerouslySetInnerHTML={{ __html: props.suggestion.description }} |
There was a problem hiding this comment.
Since this exported from the actual suggestions as @Bargs showed, couldn't we just change those suggestions classes to export a JSX element instead of a string?
getDescription() {
return (<React.Fragment>Some description that <b>could contain</b> HTML.</React.Fragment>);
}That way we would not need to use dangerouslySetInnerHTML here and could just use that description directly. I think every place we could get rid of it, would be one less place that could expose a vulnerability in the end :-)
There was a problem hiding this comment.
We definitely could, but then I think we would need to update every single consumer of these suggestions. That includes APM, beats_management, and infra, in addition to this query bar component. I'm hesitant to make that many changes to code I'm not familiar with this late in the game. Since this issue predates this PR (it was "dangerously set" in angular as well) I think it would be ok to handle it in a separate PR, do you agree?
There was a problem hiding this comment.
Yeah totally fine for me doing this in a separate PR and and making other teams aware.
|
jenkins, test this |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
|
Looks good. I have one comment that doesn't have to go in now, but that I find a bit annoying. I type in a new query and hit enter and the next thing that happens is the auto complete box pops up:
Almost seems like hitting enter should close that box and not re-open it until the user starts typing again. Otherwise I just executed that query, why do I need to see that box, and it kinda hides the filter bar. One other minor comment that I find annoying. After I select the toggle to turn on Kuery, I then click into the query bar, but the first thing that is highlighted is the Options button, not the query bar itself, so it requires two clicks to start typing. |
💚 Build Succeeded |
|
Thanks for the reviews everyone! I'm going to make a list of the non-critical suggestions to follow up on in another PR. |
Implements query bar portion of https://elastic.github.io/eui/#/layout/header. Filter bar will come in another PR. Fixes elastic#14086 Re-implements our query bar component in React using some EUI components. Existing typeahead and suggestion styles were copied over 1:1 for now after talking with Dave about it. In this PR I focused on reaching feature parity with the existing query bar. Some additional work would be needed before we could move this into EUI as a generic component that could be consumed by other plugins. Still needs some new tests and I suspect some old tests will need to be updated, but other than that this PR is functionally complete and ready for reviews.
Implements query bar portion of https://elastic.github.io/eui/#/layout/header. Filter bar will come in another PR. Fixes #14086 Re-implements our query bar component in React using some EUI components. Existing typeahead and suggestion styles were copied over 1:1 for now after talking with Dave about it. In this PR I focused on reaching feature parity with the existing query bar. Some additional work would be needed before we could move this into EUI as a generic component that could be consumed by other plugins. Still needs some new tests and I suspect some old tests will need to be updated, but other than that this PR is functionally complete and ready for reviews.
I tried out the changes and I think the missing search icon is a problem. I understand it is the new design but I think it should be changed. To "refresh" the data, you could just click the search icon. Now, I have to click into the search bar and then hit enter. There needs to be some sort of icon on the bar that reruns the query. |
|
The final K7 design with the Query, Filter and Timepicker will incorporate a "Refresh/Update" button that will take into account the "dirty" states of all three or just act as an overall refresh. Including an icon isn't obvious that you need to click it to submit the query as we use a magnify icon to prefix all search inputs but they do nothing when clicked.
@Bargs Maybe we should add this "Refresh/Update" button after the query bar now, but only have it listen to the state of the query bar for now? |
|
@cchaos, that works and it really is needed before this hits release. |
| return recentQueryString.includes(query); | ||
| }); | ||
| return matchingRecentSearches.map(recentSearch => { | ||
| const text = recentSearch; |
There was a problem hiding this comment.
@Bargs, you should also call toUser() on recentSearch if it is an object. Otherwise, react doesn't like it when the object is used in the Suggestion component.
* fixing other bucket filters (#24217) * fixing other bucket filters * adding selenium tests * using lodash flatten * using lodash flatten * fixing based on tims review * Fix functional tests * [ML] Fixes font size of headings in anomalies table expanded row (#24390) * Use metric indices when displaying metrics in the waffle map (#24251) * Add drilldown to waffle map groupings (#24301) * Resolve known APIs (#24398) * Fix: Use basepath from state for socket path (#24369) Closes #23168 This PR uses the `basePath` constant from Angular to mount the client socket instance, allowing it to connect correctly even in when using a Space. ### Workpad correctly loading in a Space  ### List of workpads restricted to a space  ### List of workpads restricted to default space  * [ML] Fixes check for lower memory limit. (#24323) - Fixes the job validation for the lower bound of the model memory limit. Previously the check was against zero, now it's again less than 1MB, which is the same as the backend expects. - If the user entered model memory limit is less than half the value of the estimated model memory limit, a warning type message gets triggered. If the user entered model memory limit is more than half the value but less then the actual value of the estimated model memory limit, then the already existing info type message is shown. The unit tests have been updated to reflect that behavior. * Euify and Reactify Query Bar Component (#23704) Implements query bar portion of https://elastic.github.io/eui/#/layout/header. Filter bar will come in another PR. Fixes #14086 Re-implements our query bar component in React using some EUI components. Existing typeahead and suggestion styles were copied over 1:1 for now after talking with Dave about it. In this PR I focused on reaching feature parity with the existing query bar. Some additional work would be needed before we could move this into EUI as a generic component that could be consumed by other plugins. Still needs some new tests and I suspect some old tests will need to be updated, but other than that this PR is functionally complete and ready for reviews. * Add latency to index and node Elasticsearch stats (#22625) Closes #22503 * [Infra UI] Adding time ranges to detail links (#24237) * [Infra UI] Adding time ranges to detail links - Adds timeranges to hosts, containers, and pods links - Fixes #24228 - Typos and what not prevented urls from working * remove argument hard coded formatting * Cleaning up query arg generation * Adding missing file * removing rison hard coded string * Make sure Vega tooltip isn't cut off (#24409) Make sure the vega options dropdown menu is visible * refactor infra folder to an ingest folder who allow multiple products under it * fix lint error * remove my temp folder * fix link for icon * rename xpack.infra or xpack/infra to xpack.ingest or xpack/ingest * update to ingest * no more infra * change graphql endpoint * fix test path * fix path for xpack infra test
|
@stacey-gammon I was just going through some of the feedback and creating issues for the minor things I didn't have time for. However I couldn't reproduce this one issue you mentioned:
Do you have any additional details about that? Browser you were using maybe? It's not happening for me in master in Safari at least. |
Implements query bar portion of https://elastic.github.io/eui/#/layout/header. Filter bar will come in another PR.
Fixes #14086
Re-implements our query bar component in React using some EUI components. Existing typeahead and suggestion styles were copied over 1:1 for now after talking with Dave about it. In this PR I focused on reaching feature parity with the existing query bar. Some additional work would be needed before we could move this into EUI as a generic component that could be consumed by other plugins.
Still needs some new tests and I suspect some old tests will need to be updated, but other than that this PR is functionally complete and ready for reviews.