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

Euify and Reactify Query Bar Component #23704

Merged
merged 54 commits into from
Oct 23, 2018
Merged

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Oct 2, 2018

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.

@trevan
Copy link
Contributor

trevan commented Oct 2, 2018

Could you "virtualize" the suggestions with something like react-virtualized?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs
Copy link
Contributor Author

Bargs commented Oct 2, 2018

@trevan was already working on it ;) 3e8ad02

@Bargs
Copy link
Contributor Author

Bargs commented Oct 2, 2018

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.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs Bargs changed the title [wip] Euify query bar Euify and Reactify Query Bar Component Oct 2, 2018
@Bargs Bargs added review Feature:Query Bar Querying and query bar features Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Oct 2, 2018
}
}

export function Suggestion(props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm mostly cool with destructuring and happy to follow whatever conventions folks are using for react stuff already, will make the update 👍

@elasticmachine
Copy link
Contributor

💔 Build Failed

@snide
Copy link
Contributor

snide commented Oct 3, 2018

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.

@Bargs
Copy link
Contributor Author

Bargs commented Oct 3, 2018

@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 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They come from the autocomplete provider types

return `<p>Filter results that contain <span class="suggestionItem__callout">${escape(fieldName)}</span></p>`;

@timroes
Copy link
Contributor

timroes commented Oct 4, 2018

Accessibility looks fine for me. aria-activedescendant is set to a value suggestion-null even if no element is selected, but since that is not a valid id, a screenreader won't break on that. Of course it would be nicer not to set it at all if no autocompletion is selected, but that's a minor nitpick (and most likely not even related to this PR).

I notice two other kind of strange behaviors, but both of them might actually also be EUI issues:

  1. If you focus the query bar, than press Tab the Options button is focused (so far so good). If you know press Enter I would expect the popover to open, but instead the search is executed, even though the Options button was focused.

  2. If you open the popover, and then click into the Query Bar, the query bar is only shortly focused, before losing its focus again.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs
Copy link
Contributor Author

Bargs commented Oct 22, 2018

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 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

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 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah totally fine for me doing this in a separate PR and and making other teams aware.

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

GLTMTLGMLTLMGLMTLGLGLGMTLMLTMLGGLMTL!

@Bargs
Copy link
Contributor Author

Bargs commented Oct 22, 2018

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor

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:

screen shot 2018-10-23 at 9 40 17 am

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.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs Bargs merged commit b99c516 into elastic:master Oct 23, 2018
@Bargs
Copy link
Contributor Author

Bargs commented Oct 23, 2018

Thanks for the reviews everyone! I'm going to make a list of the non-critical suggestions to follow up on in another PR.

Bargs added a commit to Bargs/kibana that referenced this pull request Oct 23, 2018
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.
Bargs added a commit that referenced this pull request Oct 23, 2018
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.
@trevan
Copy link
Contributor

trevan commented Oct 23, 2018

@Bargs, @cchaos

Also I noticed that there's no longer a search icon to the right of the query bar. I'm assuming this is by intention.

Yeah this is following the new design from @cchaos : https://elastic.github.io/eui/#/layout/header

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.

@cchaos
Copy link
Contributor

cchaos commented Oct 23, 2018

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.

screen shot 2018-10-23 at 19 23 17 pm

@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?

@trevan
Copy link
Contributor

trevan commented Oct 23, 2018

@cchaos, that works and it really is needed before this hits release.

return recentQueryString.includes(query);
});
return matchingRecentSearches.map(recentSearch => {
const text = recentSearch;
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

XavierM added a commit that referenced this pull request Oct 24, 2018
* 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
![screenshot 2018-10-22 15 57 00](https://user-images.githubusercontent.com/404731/47324055-251a3f80-d613-11e8-9fb0-0b0eaa3b974d.png)

### List of workpads restricted to a space
![screenshot 2018-10-22 13 49 54](https://user-images.githubusercontent.com/404731/47319741-50496280-d604-11e8-8966-83ef3a9fb320.png)

### List of workpads restricted to default space
![screenshot 2018-10-22 13 50 37](https://user-images.githubusercontent.com/404731/47319747-593a3400-d604-11e8-960d-385c80c7638a.png)

* [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
@Bargs
Copy link
Contributor Author

Bargs commented Oct 29, 2018

@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:

I type in a new query and hit enter and the next thing that happens is the auto complete box pops up

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Query Bar Querying and query bar features review Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.5.0 v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants