Support multiline Timelion queries #11972
Conversation
cjcenizal
force-pushed
the
8881/improvement/timelion-multiline-query-clean-history
branch
from
571ad8f to
9248f15
Compare
| @@ -21,7 +25,7 @@ | |||
| } | |||
|
|
|||
| start | |||
| = tree:series { | |||
| = space? tree:series { | |||
There was a problem hiding this comment.
This allows white space before the initial es call:
.es(*)
| @@ -87,7 +91,7 @@ function_name | |||
| = first:[a-zA-Z]+ rest:[.a-zA-Z0-9_-]* { return first.join('') + rest.join('') } | |||
|
|
|||
| function "function" | |||
| = space? '.' name:function_name space? '(' space? arg_list:arg_list? space? ')' { | |||
| = space? '.' name:function_name space? '(' space? arg_list:arg_list? space? ')' space? { | |||
There was a problem hiding this comment.
This allows whitespace between chained functions:
.add(1) .add(1)
.add(1)
|
CC @snide |
cjcenizal
added
Team:Platform-Design
|
The first commit introduces UI Framework styles which support an expandable query bar. These styles aren't ultimately used by the changes to the Timelion UI in this PR. We can remove these UI Framework changes, but first I want to check with the Discovery team to make sure they won't actually be useful. |
ui_framework/dist/ui_framework.css
Outdated
| * 3. Cap the height so that we can offer auto-completion suggestions below the input. | ||
| * 4. When the input is collapsed, we want text input to be truncated with an ellipsis. | ||
| */ | ||
| .kuiLocalSearchInput--expandable { |
There was a problem hiding this comment.
i see quite some duplication between this and _local_search.scss ... whats the idea behind this ?
There was a problem hiding this comment.
This is the auto-generated CSS file, so we can ignore it during review.
| } | ||
| } | ||
|
|
||
| export function suggest(expression, caretOffset, functionList, Parser) { |
There was a problem hiding this comment.
could you fix this function so it also shows the suggestion after you start writing your arguments in ?
.es ... shows me the doc for es function with all the arguments
after i type .es( the arguments disappear which is supper annoying.
i understand if you want to do it in separate pr but it might be a small change ....
maybe storing last valid caret position and in just showing the doc for that ?
| @@ -53,6 +58,38 @@ | |||
| } | |||
|
|
|||
| /** | |||
| * 1. When it expands, the search input should lie on top of the content. | |||
There was a problem hiding this comment.
i guess this comments are no longer completely relevant ?
| * 2. Expand as the user enters text. | ||
| * 3. Cap the height so that we can offer auto-completion suggestions below the input. | ||
| * 4. When the input is collapsed, we want text input to be truncated with an ellipsis. | ||
| * 5. Prevent textarea from being resized. |
There was a problem hiding this comment.
not sure if you wanted to prevent from resizing textarea by height ... i can still expand it
| @@ -53,6 +58,38 @@ | |||
| } | |||
|
|
|||
| /** | |||
| * 1. When it expands, the search input should lie on top of the content. | |||
| * 2. Expand as the user enters text. | |||
There was a problem hiding this comment.
input does not auto grow for me
| @@ -53,6 +58,38 @@ | |||
| } | |||
|
|
|||
| /** | |||
| * 1. When it expands, the search input should lie on top of the content. | |||
| * 2. Expand as the user enters text. | |||
| * 3. Cap the height so that we can offer auto-completion suggestions below the input. | |||
There was a problem hiding this comment.
i can expand the textarea as far as i want (multiple pages) .. the cap does not seem to be working
| * 1. When it expands, the search input should lie on top of the content. | ||
| * 2. Expand as the user enters text. | ||
| * 3. Cap the height so that we can offer auto-completion suggestions below the input. | ||
| * 4. When the input is collapsed, we want text input to be truncated with an ellipsis. |
There was a problem hiding this comment.
no ellipsis, just the scrollbars
ui_framework/dist/ui_framework.css
Outdated
| @@ -1915,6 +1932,43 @@ body { | |||
| /* 1 */ } | |||
|
|
|||
| /** | |||
| * 1. When it expands, the search input should lie on top of the content. | |||
There was a problem hiding this comment.
this seems to be duplicated code ? or ?
| @@ -125,6 +128,29 @@ export default props => ( | |||
| </GuideSection> | |||
There was a problem hiding this comment.
irrelevant ... any idea why does github color this lines red ?
There was a problem hiding this comment.
My guess is that it doesn't recognize JSX or, if it does, it's trying to highlight invalid syntax because of the closing tag at the top?
cjcenizal
force-pushed
the
8881/improvement/timelion-multiline-query-clean-history
branch
from
9248f15 to
57cd126
Compare
|
@ppisljar I updated the PR so the suggestions don't disappear until the expression's been completed. Can you take another look? |
|
@cjcenizal I had a chance to take this PR for a spin. Some thoughts and comments below:
I'll continue to play around with this but please let me know you'd like to discuss any of these in more detail. Regardless of my feedback, what we have here is already a significant improvement for lengthy queries 😄 |
|
@cjcenizal what about my other comments ? |
|
the auto expand/collapse functionality would make this much more user friendly imo. As far as i notice this is pretty much the biggest difference from the previous implementation with div. However looking in the future it would be nice if at some point this would be much more like the 'console' editor .... (seems the suggestions work well there as well) so i am having second thoughts on the textarea. I agree with Alex that, in any case, we should make this feel more like the console (same key shortcuts) |
There was a problem hiding this comment.
Pretty nice, I like it! No major comments so far.
wrt. @ppisljar However looking in the future it would be nice if at some point this would be much more like the 'console' editor .... (seems the suggestions work well there as well) so i am having second thoughts on the textarea.
I assume this refers to things like syntax color-coding, inline-autosuggest. If so, IMO that doesn't need to be part of this PR as multi-line addition is already quite nice, and full-fledged editor support can come later.
I do agree with this earlier feedback from @alexf and @ppisljar.
- autoexpanding the textarea-box as-you-type
- using the cmd-enter combo to match the console
|
This is great feedback, peeps! I'll implement the following:
|
|
OK, so I ran into a couple issues trying to get the "peeking" behavior @alexfrancoeur suggested (i.e. expanding the textarea when you focus it). You can see my attempt in 95f602d. First of all, I think we want to disable this behavior once the user has resized the textarea, my assumption being that you resize the textarea so you can see its contents better and it'd be really annoying for it to expand and collapse as you focus and blur it. However, because there's no default "resize" event, I had to hack together a way of deriving whether the user has resized the textarea or not. I got that figured out, but then there's a ridiculous Chrome bug which prevents the textarea from being resized below the "peek" height, but only if you resize it while its focused. This only happens in Chrome and at first I thought I was going crazy. Not only does Chrome provide the incorrect UX, it does so consistently, because it will work fine if you resize the textarea before you've clicked on it, but it will not work correctly if you resize it after you've clicked on it. If our goal is simply to teach users that they can enter multiline expressions, I figure why not just use the UI to communicate that without getting too fancy?
Thoughts? @alexfrancoeur Let's wait on adjusting the empty gray area in the local nav until we've merged @snide's color contrast changes in #11610. It will be easier to make adjustments after we get those changes in because the local nav's height will no longer correspond to the height of the logo in the side nav. If we adjust the height of that local nav now, it will look a little weird and inconsistent, due to the way those two heights are currently visually related. Changing local nav code will also affect all apps, so we should do that in a separate PR. |
cjcenizal
force-pushed
the
8881/improvement/timelion-multiline-query-clean-history
branch
from
86d7504 to
bd23f2c
Compare
|
@thomasneirynck i agree that doesn't need to be part of this PR, i was just saying that it would be nice that when we decide to do that we don't need to throw all this work away, so keeping that in mind here and not locking us down to much is important imo. @cjcenizal i like what you did there! |
|
@cjcenizal regarding the console-like submit - this definitely makes it easier to add multiple lines, so ++ on this. Should we add "keyboard tips" in the docs here similar to console? I don't think we'd have as much but an indent option would be nice to have here as well
Making the input box multiple lines makes it more obvious that you can enter a longer query, but I still prefer something along the lines of #11975 for the user experience. What about something like this example? We could add a limit of 5 - 10 rows for auto-expand but a user can always drag further if they'd like. If a user clicks on a timelion panel that has a long query or if a user copy and pastes a long query, the input box will update to the appropriate or default maximum height. They would still have the ability to expand further by dragging and dropping. Also I think it's fine to wait for #11610 before improving the top menu. |
…it won't overlap with graphs when focused.
- Make suggestions popover charts instead of pushing the down. - Align controls to top of expression input if the textarea is resized to be tall.- Tweak interval styles to look nice.
… already resized it.
…port multiple lines, by having a taller min-height.
cjcenizal
force-pushed
the
8881/improvement/timelion-multiline-query-clean-history
branch
from
f879b32 to
35fefff
Compare
There was a problem hiding this comment.
Nice! Some minor comments for your consideration.
I've googled some example of auto-resize and, similar to @alexfrancoeur's link, on Chrome, all of them stayed expanded, and none of them allowed users to shrink the box back. This is an interesting problem, but will this genuinely impact our users? The visualization does not disappear, it just gets pushed lower and lower on the screen. This appears to me a fair trade-off.
So I don't think the inability to shrink the textarea back to a size smaller than the content, would actually be problematic for our users.
I don't feel strongly about either though, so I'll go with the majority opinion on this one.
| template: timelionExpressionInputTemplate, | ||
| link: function (scope, elem) { | ||
| const expressionInput = elem.find('[data-expression-input]'); | ||
|
|
There was a problem hiding this comment.
consider extracting this enumeration to new module. we have the exact same enumeration in https://github.com/elastic/kibana/blob/master/src/ui/public/typeahead/typeahead.js#L10.
| } | ||
|
|
||
| function setCaretOffset(caretOffset) { | ||
| // Wait for Angular to can update the input with the new expression and *then* we can set |
ui_framework/components/_mixins.scss
Outdated
| @mixin formControl($borderRadius: $globalBorderRadius) { | ||
| @include formControlBase; | ||
| background-color: #ffffff; | ||
| border: 1px solid $globalFormControlBorderColor; | ||
| border-radius: $borderRadius; | ||
| transition: border-color $globalInputTransitionTiming; | ||
| min-height: 30px; /* 1 */ |
There was a problem hiding this comment.
not an expert on sass and the kui, but since this height is interdependent, can this 30px be made a variable?
|
UX wise: this is possible to use and gives you something you didn't have before (multiline editor) ... but, the editor doesn't give you really good experience ... so not sure if this counts as 'baseline satisfactory UX' ... it does solve the issue, so it could be. I personally would love to see it improved, but i am having doubts if its worth the effort. UI wise: this might look a bit 'less cool' that the rest of kibana (specially the new parts the design team is improving on !!) however it falls quite well into the whole timelion look&feel |
|
@cjcenizal the keyboard section looks great! I'd almost say it's fair to use the when auto-complete is visible section as well. This seems to be applicable for timelion functions when auto-complete is triggered.
If the input box doesn't have the ability to shrink back then switching between panels can become a awkward. You could imagine switching between a 10 line expression and a single line expression. As I see it, this could behave in two ways. We either push the panels down based on the length of the query and transform the sheet each time you focus on a panel or we have a ton of empty space on the 1 line expression to keep the sheet consistent. Neither of which seem like a good solution to me. I'm with Peter here. I think it stops the bleeding but isn't necessarily the best UX. This is my last pitch I swear 😄 What if we restricted this a bit more and made it an opt-in experience? Rather than allowing for a custom / dynamic input box, what if we defined a minimum and maximum height - giving the user the option to keep it collapsed or expand. To me, this gives me the feeling that I'm entering a workspace for creating timelion expressions. The control is no good here but I thought this might help visualize my idea.
|
|
@thomasneirynck @ppisljar @snide I met up with @alexfrancoeur and we agreed that there's room for improvement in the UX, but because a) this PR provides functionality users have been asking for, b) the UX improvements depend upon a hack, and c) this PR targets and alpha release, we should merge this and get user feedback while I work on implementing the hack for improving the UX. So my next steps will be: implementing Thomas's feedback and securing code review approvals from him and Peter, creating an issue to define the improved UX and identify the hack needed to implement it, and then merging this PR. |
|
@thomasneirynck I've addressed your feedback, and @alexfrancoeur I added the Help tips you suggested:
Could you please re-review? |
|
@cjcenizal LGTM - let's make sure to reference the issue that will tackle the improved UX here as well. Maybe update the description once it's created? |
cjcenizal
deleted the
8881/improvement/timelion-multiline-query-clean-history
branch
|
Thanks so much for this PR! |
Supersedes #11164 with a cleaner history.
Addresses #8881 and #11449.