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

EuiSelectableTemplateSitewide for Elastic's global search #3800

Merged
merged 36 commits into from
Aug 18, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jul 27, 2020

This PR sets up a template wrapper on top of EuiSelectable that creates an opinionated pattern for create Elastic's global/sitewide search.

Screen Shot 2020-08-11 at 16 20 47 PM

The documentation example describes it best, so I refer those reviewing to read that portion of the docs.

I also added an example of how this fits into the global header, including a quick (ie hacky) style override to change the non-focus input into a dark version while sitting within a dark header.

Screen Recording 2020-08-11 at 04 24 45 PM


Other component updates

EuiBadge

Prior to this PR, if a consumer provided only an icon and no text (which was allowed), the layout would still include margins as if there were an icon and text next to each other. This PR updates the render of the EuiBadge contents so that the text wrapper only appears if there is children and updates the styles to match. Also, since then the icon was so tiny that it made the badge size too small, I increased the size of the icon back to m but only if there is no children.

Screen Shot 2020-08-11 at 16 36 54 PM

This also then updated the usage of the enter badge for EuiComboBox.

EuiMark and EuiHighlight

Syntactically, EuiHighlight should have used EuiMark as the renderer of the highlighted portion of text, but EuiMark came in late. This PR updates EuiHighlight from rendering a <strong> tag to rendering a <EuiMark> (<mark>) tag.

Also, when EuiMark was created, it wasn't actually styled and was only using the browser's default render of the <mark> tag. I updated the style of EuiMark to match that of the original <strong> tag, essentially just bolding it. I also updated the Amsterdam background color of EuiMark to our light blue.

Before vs After Screen Shot 2020-08-11 at 16 38 39 PM Screen Shot 2020-08-11 at 16 38 14 PM

EuiSelectable

I've added a few more options to this component that I've also documented in the various sections of the docs.

Options props

I added a couple extra capabilities to the option object itself.

  • searchableLabel: This provided the sitewide template to be able to include the meta data in the searchable context, but not use a separate display for the label.
  • [key: string]: any: We've seen consumers ask that our data objects be more extendable. The addition of this generic key/value prop should allow consumers to pass their full data object instead of a modified version of.

Custom messages

The component used to explain that consumers would have to replace the render of list with their custom message, but this was too manual because the consumer would have to do a check for each state. Now the component provides loadingMessage, noMatchesMessage, and emptyMessage that just accept nodes to replace those messages with.

EuiSelectableMessage also now accepts a bordered prop in order to match the render of the list.

listProps.onFocusBadge

Similar to the new EuiComboBox enter badge, EuiSelectable's list items will also show an enter badge on focus. This prop allows consumers (like the sitewide template) to override the appearance of this badge.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • [ ] Checked in mobile I will do a follow up PR that addresses the mobile header version
  • Checked in Safari and Firefox
  • Props have proper autodocs
  • Added documentation
  • [ ] Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

@ryankeairns
Copy link
Contributor

ryankeairns commented Jul 29, 2020

I've started to take a look at this (in Kibana) alongside John's mockups and I'm wondering how we should approach feedback/tweaks.

For example, let's suppose we wanted to change the background color of the unfocused search input, or maybe we want to change the width of the form layout. Would we do that on the EUI side? perhaps within the header component and force dark input styles? or, should we address such design overrides with custom CSS on the Kibana side?

@cchaos
Copy link
Contributor Author

cchaos commented Jul 29, 2020

I know what you're referencing is specifically changing the styles of the search input to look better on the black header. That will be coming later and will most likely need to happen on the EUI side as it will require quite a bit of style overrides that we'll want consistent in Kibana, Cloud, etc.

As for the width of the form element, that is completely controllable by the location by the consuming application and any searchProps you may pass down to it.

I originally posted this PR up just to give a heads up that this is the direction that I'm taking from the EUI side, so if you need more than what it provides, please explain in detail. Of course this is completely lacking any documentation as it is sill a draft which would have probably helped you out figuring what you might need. But consider that the component basically takes all the same props as the basic EuiSelectable component, so you can use those docs as examples.

@ryankeairns
Copy link
Contributor

I know what you're referencing is specifically changing the styles of the search input to look better on the black header. That will be coming later and will most likely need to happen on the EUI side as it will require quite a bit of style overrides that we'll want consistent in Kibana, Cloud, etc.

Good to know, I wasn't aware there would be another PR. We'll do what we can with the props and then sync up after the final EUI PR.

I originally posted this PR up just to give a heads up that this is the direction that I'm taking from the EUI side, so if you need more than what it provides, please explain in detail.

I'll keep this initial feedback mostly geared toward content and UX within the context of Kibana. That should help us scare up any missing pieces (nothing jumped out on an initial pass).

Thanks!

@ryankeairns
Copy link
Contributor

I provided some feedback on @myasonik 's PR that seem to only occur when I was using this branch (+ the related Kibana implementation PR). It's not clear if anything needs to be done, but things to consider:

  • The keyboard shortcut works on Michail's base branch, but not when this build of EUI is applied. FWIW, I notice it works in the EUI docs, but only upon release of the Command key (as opposed to initial press).
  • Similarly, tabbing into and out of the search input works on the base branch, but not after this branch is applied. Tabbing does work as expected in the EUI docs example, so maybe just something to keep an eye on.
  • The results panel flickers while on this branch/build... not sure why. Probably not related, I also see the height in the EUI docs example is dynamic while it is fixed on the Kibana side.

@cchaos
Copy link
Contributor Author

cchaos commented Aug 5, 2020

The keyboard shortcut

Yeah, this is totally an implementation detail, and is not baked into the template component. The docs example will have something wired up primarily as an example. But because this could be really specialized in how applications (outside Elastic) want to handle such a keyboard shortcut, it costs too much to handle all the specificities than just giving consumers an example.

Tabbing into and out of the search input

When you say this, what do you mean by works/doesn't work. The auto-showing of the popover, the actual focus of the input, something else? I'd be interested in investigating why there is such a difference, but my guess is that it has to do with the placement of the search input in the header.

results panel flickers

I'm pretty sure this happens because of the way the Kibana side does the search. In the EUI docs, the component is given all possible options to do a text-based search on. While on the Kibana side, each time a user enters a letter to search, Kibana has to ping a separate API which takes time, then it passes back the results to the EUI component, essentially by-passing EuiSelectable's built-in search functionality.

During this API ping, @myasonik is putting the EuiSelectable component into the isLoading state which replaces the list entirely with the loading message. He and I briefly talked about this, so I think he is aware that he'll need to implement something to better handle the intermittent states.


Overall I do think your concerns mainly lie with Kibana's implementation.

Some of the other pieces to review would be:

  • Does the options object format work with the search API?
  • Does this fill all the needs for formatting each of the options (coloring meta data and such)?
  • Are all the aesthetics what we need? Like the hover, focus and other states of the list itself.

@ryankeairns
Copy link
Contributor

ryankeairns commented Aug 5, 2020

Tabbing into and out of the search input

When you say this, what do you mean by works/doesn't work. The auto-showing of the popover, the actual focus of the input, something else? I'd be interested in investigating why there is such a difference, but my guess is that it has to do with the placement of the search input in the header.

While purely on Michail's PR, I can tab across the header from Elastic Logo > search input > clear input > support button.
When I build EUI from this EuiSelectableTemplateSitewide PR, that tabbing no longer works on the Kibana side.

Update: this and the keyboard shortcut both work on the Gatsby prototype so 🤷‍♂️ - https://cchaos.github.io/kibana-8-nav/ Is that using the EuiSelectableTemplateSitewide? Just trying to narrow down if this new component has any bugs in this regard.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

2 similar comments
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

Copy link
Contributor Author

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I'm still working out some of the CI failures (a11y), but @thompsongl can you help me with a TS issue? I'll comment below.

src-docs/src/views/selectable/search.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

@cchaos
Copy link
Contributor Author

cchaos commented Aug 13, 2020

Ok that last commit should fix CI (Axe) tests. This is now ready for review :)

@cchaos cchaos marked this pull request as ready for review August 13, 2020 19:02
@cchaos cchaos requested a review from thompsongl August 13, 2020 19:03
@cchaos cchaos changed the title [WIP] EuiSelectableTemplateSitewide EuiSelectableTemplateSitewide for Elastic's global search Aug 13, 2020
@cchaos cchaos requested a review from elizabetdev August 13, 2020 19:05
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

@myasonik
Copy link
Contributor

Screen Shot 2020-08-13 at 22 13 20

Compared to the screenshot in your description, this seems like a bug? (Search input should turn white on focus?)

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

Worth the wait!

@cchaos
Copy link
Contributor Author

cchaos commented Aug 14, 2020

@myasonik The input will only turn back to white in light mode. In dark mode, we keep the original styling dark.

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Wow! It looks great. 🎉

Tested in Safari, Firefox, and Edge. I also looked at the code and everything looks good.

I just found one misspelling when running the text in Grammarly (I'm not completely sure it is really a misspelling but I added it as a suggestion).

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

1 similar comment
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

@ryankeairns
Copy link
Contributor

Thanks for keeping this moving along!

Once this is merged, and Kibana has been updated to include it, we'll then move the header/search PR out of draft mode. @thompsongl if you could take a look soon, it would be very much appreciated as opening Michail's PR is our next major milestone.

cchaos and others added 2 commits August 18, 2020 13:50
@cchaos cchaos requested a review from thompsongl August 18, 2020 17:57
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

src/ code changes LGTM

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3800/

@cchaos cchaos merged commit df6dd76 into elastic:master Aug 18, 2020
@ryankeairns
Copy link
Contributor

tenor-183052315

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants