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

Add search to formula #21

Conversation

flash1293
Copy link

Adds a search input to the formula help

@MichaelMarcialis
Copy link

Reviewing now. Will provide a design PR with some of my suggestions shortly.

@MichaelMarcialis
Copy link

Hey, @flash1293. Per my previous comment, I've made a quick design PR for this branch to tweak some of the styles. Please review here when you get a moment: flash1293#13

Additionally, a few questions came up during my review of this PR:

  1. I noticed that the headings for the docs' nav groups are always showing, regardless the currently entered search string returns no matching child functions for that group and that string is not contained within the heading. Should these headings be shown given those circumstances? My initial instinct is that we shouldn't be showing them in that case.

image

  1. While individual function items are given an EuiMark in order to show the portion of the search string contained in each item, no such indicator is provided to nav group headings. Should the matching search strings within the nav group headings be stylistically offset to show the matching search string like we do for function items?

image

@flash1293
Copy link
Author

@MichaelMarcialis Made the section headers searchable as well (including hiding them if nothing in a section matches current search)

Screenshot 2021-05-26 at 14 12 41

Copy link
Owner

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

@flash1293 LGTM, just curious about the markdown switch.

} from '@elastic/eui';
import { Markdown } from '../../../../../../../../../src/plugins/kibana_react/public';
Copy link
Owner

Choose a reason for hiding this comment

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

I thought we agreed on EuiMarkdownFormat @flash1293?

@wylieconlon wylieconlon merged commit f7bcefd into wylieconlon:lens/formula-error-handling May 26, 2021
@MichaelMarcialis
Copy link

@flash1293 LGTM, just curious about the markdown switch.

The EuiMarkdownFormat component doesn't appear to offer any control over the text styling, and wrapping it inside an EuiText component was yielding inconsistent results in some instances. It was more controllable and easier to maintain by using the Markdown component instead and wrapping the entire output in a single EuiText.

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.

3 participants