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 visual indication of selected language in query bar #30899

Merged
merged 12 commits into from
Feb 20, 2019

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Feb 12, 2019

Summary

Fixes #30277

screen shot 2019-02-12 at 6 00 53 pm

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@Bargs Bargs added Feature:Query Bar Querying and query bar features v7.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.2.0 labels Feb 12, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

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.

LGTM, you might consider removing a little bit of the padding (5px or so) since it's a little smaller now

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs Bargs requested a review from a team as a code owner February 12, 2019 23:51
@Bargs
Copy link
Contributor Author

Bargs commented Feb 12, 2019

@lukasolson good point, I made the padding a little smaller

@cchaos
Copy link
Contributor

cchaos commented Feb 13, 2019

@Bargs I think we can remove that container around the language selector and add it directly as an append to the EuiFieldText like:

append={<QueryLanguageSwitcher
                        language={this.state.query.language}
                        onSelectLanguage={this.onSelectLanguage}
                      />}

This way, the actual input will always size correctly depending on the size of the language switcher button.

image

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Functionality looks good, and I think having the active syntax showing is fine. Only thing I'd touch up along with this is the text, otherwise "KQL" without context doesn't have meaning. These features are also no longer new, so it should just state what the toggle does. @gchaps can you provide text for this popover to Matt please. I'd suggest something like:

Use the Kibana Query Language (KQL) syntax to enable autocomplete suggestions. If disabled, Kibana will use Lucene syntax without suggestions.

SWITCH Turn on autocomplete

image

Otherwise tested and approved once the text is changed.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger
Copy link
Contributor

spalger commented Feb 13, 2019

@gchaps
Copy link
Contributor

gchaps commented Feb 13, 2019

Here's a recommendation for the text:

Use the Kibana query language (KQL) to get suggestions as you type. If you disable KQL, Kibana uses Lucene, which doesn't offer suggestions.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs
Copy link
Contributor Author

Bargs commented Feb 14, 2019

@snide @cchaos @gchaps I've made all the requested updates:

screen shot 2019-02-14 at 5 37 56 pm

@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs
Copy link
Contributor Author

Bargs commented Feb 14, 2019

Opened PR to add the append attribute to EuiFieldText's TS definition elastic/eui#1567

@gchaps
Copy link
Contributor

gchaps commented Feb 14, 2019

I have a few suggestions:

  • Change "See docs here" to "See KQL docs."

  • To make the language in the intro text and toggle text consistent, change "If you disable KQL" to "If you turn off KQL"

  • Remove the text "Not ready yet? Find our Lucene docs here." I don't think it is needed anymore.

@Bargs
Copy link
Contributor Author

Bargs commented Feb 15, 2019

@gchaps made those updates, but I realized one other change we'll need to make. Autocomplete only works with a basic license and above. OSS users still get the other benefits of KQL (scripted field support, simplified syntax) but they don't get autocomplete, so the the current message will probably be quite confusing to OSS users. How do you think we could resolve that?

screen shot 2019-02-14 at 8 03 48 pm

@elasticmachine
Copy link
Contributor

💔 Build Failed

@gchaps
Copy link
Contributor

gchaps commented Feb 15, 2019

@Bargs Here is a suggestion:

The Kibana query language (KQL) offers a simplified query syntax and support for scripted fields. KQL also provides autocomplete if you have a Basic license or above. If you turn off KQL, Kibana uses Lucene.

(Hopefully that's not too long.)

Also, I noticed that the toggles on the Advanced settings page are set up like this and was wondering if we might do the same here. Its a little more clear about what's being turned on and off:

screen shot 2019-02-15 at 7 57 43 am

So in this case:

Kibana query language
Toggle button On/Off

@Bargs
Copy link
Contributor Author

Bargs commented Feb 15, 2019

@gchaps let me know what you think

screen shot 2019-02-15 at 12 27 00 pm

@elasticmachine
Copy link
Contributor

💔 Build Failed

@gchaps
Copy link
Contributor

gchaps commented Feb 15, 2019

The text looks good to me. I think its easier to understand what the toggle does when it says On & Off, but I'll defer to @snide's comment.

@Bargs
Copy link
Contributor Author

Bargs commented Feb 15, 2019

Ok so just so I'm sure I understand, @snide you want me to change it back to this?

screen shot 2019-02-15 at 3 21 18 pm

@snide
Copy link
Contributor

snide commented Feb 15, 2019

I was good with what you had. Mergable.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs Bargs merged commit cd7e1df into elastic:master Feb 20, 2019
Bargs added a commit to Bargs/kibana that referenced this pull request Feb 20, 2019
* Add visual indication of selected language to the query bar

* update test snapshots

* Reduce padding a little to fit smaller text

* Pass options button to input's append prop so we don't have to manage
the padding manually

* Revert "Reduce padding a little to fit smaller text"

This reverts commit 8042b09d

* Update popover text

* delete options button translation key

* Update tests

* Review feedback

* Review feedback

* remove unused vars

* remove unused translation
Bargs added a commit that referenced this pull request Feb 20, 2019
)

* Add visual indication of selected language to the query bar

* update test snapshots

* Reduce padding a little to fit smaller text

* Pass options button to input's append prop so we don't have to manage
the padding manually

* Revert "Reduce padding a little to fit smaller text"

This reverts commit 8042b09d

* Update popover text

* delete options button translation key

* Update tests

* Review feedback

* Review feedback

* remove unused vars

* remove unused translation
Bargs added a commit to Bargs/kibana that referenced this pull request Feb 25, 2019
* Add visual indication of selected language to the query bar

* update test snapshots

* Reduce padding a little to fit smaller text

* Pass options button to input's append prop so we don't have to manage
the padding manually

* Revert "Reduce padding a little to fit smaller text"

This reverts commit 8042b09d

* Update popover text

* delete options button translation key

* Update tests

* Review feedback

* Review feedback

* remove unused vars

* remove unused translation
Bargs added a commit that referenced this pull request Feb 26, 2019
)

* Add visual indication of selected language to the query bar

* update test snapshots

* Reduce padding a little to fit smaller text

* Pass options button to input's append prop so we don't have to manage
the padding manually

* Revert "Reduce padding a little to fit smaller text"

This reverts commit 8042b09d

* Update popover text

* delete options button translation key

* Update tests

* Review feedback

* Review feedback

* remove unused vars

* remove unused translation
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 release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants