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

Canvas function/argument/value autocomplete #23200

Merged
merged 17 commits into from
Oct 18, 2018

Conversation

lukasolson
Copy link
Member

This PR updates the Canvas expression input with autocomplete capabilities, so that functions, arguments, and in some cases even values can be suggested. When they are suggested and highlighted, we show the corresponding reference when available.

autocomplete

Note that this PR updates the arg definition to have an options array, which are what will be suggested for values. In the future we'd like to make this more robust and provide a more flexible way to generate suggestions, but this works for now.

@lukasolson lukasolson added WIP Work in progress v6.5.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Sep 14, 2018
@lukasolson lukasolson self-assigned this Sep 14, 2018
@w33ble
Copy link
Contributor

w33ble commented Sep 15, 2018

There's something funny about the way this is handling quoted values.

sep-14-2018 17-04-26

If I provide a value without quotes, the suggest stays open, but if I quote the value, it goes away. It does come back if I close the quotes and then go back into the value and start changing it though.

@ryankeairns
Copy link
Contributor

ryankeairns commented Sep 17, 2018

The functionality feels good, nothing is immediately jumping out at me in that regard. I was considering whether we want to highlight the first item as soon as the autocomplete box appears (Slack does this), but the difference here is that the return/enter key is used to start a new line in the expression builder whereas in Slack you tend to work on the current line and use Enter to submit. Point being, I think what we have is preferable for our use case and I'm only noting this here to capture the thought for posterity's sake.

There are a couple of minor visual tweaks that could be made - the mockups used a slightly different background color to differentiate the two panels, some spacing and font styling tweaks here and there, and we could probably shorten the height of the entire box once you narrow down to a single function or argument - but those could be done in a follow-up and shouldn't block this PR from moving along, in my opinion.

LGTM!

@alexfrancoeur
Copy link

Took this for another quick spin this morning and had some feedback.

filters doesn't seem to be a suggested function

screen shot 2018-09-21 at 9 15 03 am

I clicked on type= and got this error

screen shot 2018-09-21 at 9 15 22 am

It seems odd to me that we aren't showing anything on the right hand side and not necessarily intuitive that I need to click on the function to get a description of it. Should we automatically highlight the first one?

screen shot 2018-09-21 at 9 15 41 am

Is this a standard format for providing argument suggestions? I'm a bit further removed from code editors these days but I'm wondering if there is a better way to represent the argument details. @ryankeairns what do you think?

screen shot 2018-09-21 at 9 16 04 am

@ryankeairns
Copy link
Contributor

ryankeairns commented Sep 21, 2018

@alexfrancoeur see my comment just prior to yours regarding highlighting the first item - it feels like a nice thing but I think it could bring some issues with regards to our use of multi-line expressions. We could try it out, but I suspect it could lead to a path where you end up needing to use Shift+Enter to start a new line. For our scenario, that seems like it would become cumbersome if you're wanting a new line and its suggesting the next thing (i.e. interpreting the Enter might get tricky).

As an alternative (image below), we could display some higher level/more generic info on the right side when nothing is highlighted on the left side. Something to consider there is that the info would display over and over and would likely become not useful, quickly. Another option would be to not have it as wide, initially, and then expand once an item is highlighted.

image 2018-09-21 at 9 00 42 am

As for the argument display, some of them can have long descriptions, so we (Rashid, Lukas, and I) discussed displaying them as fullwidth/single column as Lukas has done here (and as you see in TimeLion). I also had a note about this in my previous comment that I'd like to touch this up visually.

Suffice it all to say, I think we should get this PR merged (once ready from a UX/technical standpoint) and I'll follow up with a PR for some UI enhancements.

@lukasolson
Copy link
Member Author

@alexfrancoeur

filters doesn't seem to be a suggested function

Yeah, public functions aren't being suggested now, it's a bug related to other stuff. I believe @rashidkpc is looking into that.

I clicked on type= and got this error

I'm not seeing this... Do you have steps to reproduce this consistently?

@ryankeairns
Copy link
Contributor

FYI - I'm about to make some design tweaks and will push them up here when ready.

@ryankeairns
Copy link
Contributor

@lukasolson I added some small design changes and opened a PR against your forked repo/branch 👉 lukasolson#16

@lukasolson lukasolson changed the title [WIP] Canvas function/argument/value autocomplete Canvas function/argument/value autocomplete Oct 4, 2018
@lukasolson lukasolson added review and removed WIP Work in progress labels Oct 4, 2018
@lukasolson
Copy link
Member Author

Still working through writing tests but I think I've cleaned up the code enough that you can take a look.

@ryankeairns
Copy link
Contributor

We're being pretty stingy with the vertical space. The PR I put up just worked to minimize that, so let's keep it as-is for the time being. Looking at that screenshot, I'm not sure my prior changes were in the branch tested... I added some space between those values to make them easier to read.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alexfrancoeur
Copy link

Took it for a spin again, LGTM. Haven't run into any big issues. I'll leave the same feedback I did in slack. It might be nice to add a turn off autocomplete option that is user specific and stored locally. In case there is an issue, any user can turn this off and (hopefully) open an issue in the Kibana repo.

@ryankeairns
Copy link
Contributor

ryankeairns commented Oct 18, 2018

@alexfrancoeur @lukasolson we have some free whitespace down in the lower left hand corner of the expression editor panel where we could place an EuiSwtich to toggle on/off the autocomplete feature:

screenshot 2018-10-18 08 52 00

Regardless of how it persists (per workpad/across workpads; presuming the latter), keeping it close and in context to the expression editor input itself feels like the best approach, imo.

Side note: this is using the compressed prop which is the shorter version of this component.

@alexfrancoeur
Copy link

alexfrancoeur commented Oct 18, 2018 via email

@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elastic elastic deleted a comment from elasticmachine Oct 18, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukasolson lukasolson merged commit 314d1c4 into elastic:master Oct 18, 2018
lukasolson added a commit that referenced this pull request Oct 22, 2018
* feat: canvas autocomplete

* fix: remove unused files

* autocomplete ui cleanup

* fix: handle stuff inside quotes

* fix: canvas suggestion comparator

* fix: spaces at the beginning of expressions

* fix: move header out of autocomplete component itself

* fix: add tests

* fix: failing test

* fix: pointed to wrong module
@lukasolson
Copy link
Member Author

6.x (6.5.0): 7ff4f81

@lukasolson lukasolson deleted the feat/canvas/autocomplete branch October 31, 2018 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants