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

Refactor Timelion expression suggestion logic for clarity. #11285

Merged
merged 5 commits into from May 4, 2017

Conversation

Projects
None yet
3 participants
@cjcenizal
Contributor

cjcenizal commented Apr 17, 2017

Extracts some work from #11164.

Overview

The current version of this logic involves DOM manipulation from within a directive. We can simplify this by leveraging Angular's directives.

Changes

  • Create timelionExpressionInput directive.
  • Create timelionExpressionSuggestions directive.
  • Create expression_directive_helpers service (which can be unit tested).
@rashidkpc

This comment has been minimized.

Show comment
Hide comment
@rashidkpc

rashidkpc Apr 17, 2017

Member

This comment isn't a review of this pull, just a good place to leave some notes about suggestions.

Timelion's autocomplete has always been one of those "Well, its better than nothing" sort of things. There's a quick digest of how I'd like to see autocompletion work here: https://github.com/cjcenizal/kibana/blob/ed3862e0abb35a2e7e69355d6993a0af166de3ea/src/core_plugins/timelion/public/directives/expression_directive.js#L14-L37

This pull should help with that process as it breaks out the logic for finding and inserting into the helpers file. Just needs someone to sit down and sort it out. Its probably not that hard to implement, the AST already has all of the lookup objects to make determining context for caret position simple.

{
   "tree": [...],
   "functions": [
      {
         "type": "function",
         "function": "es",
         "arguments": [...],
         "location": { // Encompasses the name of the function and all arguments names, and values passed to it.
            "min": 1,
            "max": 15
         },
         "text": ".es(index=bar)"
      }
   ],
   "args": [
      {
         "type": "namedArg",
         "name": "index",
         "value": {
            "type": "literal",
            "value": "bar",
            "location": { // Contains only the range of chars representing the argument value
               "min": 11,
               "max": 14
            },
            "text": "bar"
         },
         "location": { // Both the argument value, and its name, and the = sign.
            "min": 5,
            "max": 14
         },
         "text": "index=bar",
         "function": "es"
      }
   ],
   "variables": {}
}

Basically we just need to take the caret position and compare it to the location.min and location.max in each object in functions and args. That should give you an idea of if the user is typing a new function, new argument or an argument value. Once you know that you could use one of 3 suggester method to know what to pass to insertAtLocation. The only method currently implemented is for functions, but it would be trivial to add the user 2 once the caret context thing is sorted.

Member

rashidkpc commented Apr 17, 2017

This comment isn't a review of this pull, just a good place to leave some notes about suggestions.

Timelion's autocomplete has always been one of those "Well, its better than nothing" sort of things. There's a quick digest of how I'd like to see autocompletion work here: https://github.com/cjcenizal/kibana/blob/ed3862e0abb35a2e7e69355d6993a0af166de3ea/src/core_plugins/timelion/public/directives/expression_directive.js#L14-L37

This pull should help with that process as it breaks out the logic for finding and inserting into the helpers file. Just needs someone to sit down and sort it out. Its probably not that hard to implement, the AST already has all of the lookup objects to make determining context for caret position simple.

{
   "tree": [...],
   "functions": [
      {
         "type": "function",
         "function": "es",
         "arguments": [...],
         "location": { // Encompasses the name of the function and all arguments names, and values passed to it.
            "min": 1,
            "max": 15
         },
         "text": ".es(index=bar)"
      }
   ],
   "args": [
      {
         "type": "namedArg",
         "name": "index",
         "value": {
            "type": "literal",
            "value": "bar",
            "location": { // Contains only the range of chars representing the argument value
               "min": 11,
               "max": 14
            },
            "text": "bar"
         },
         "location": { // Both the argument value, and its name, and the = sign.
            "min": 5,
            "max": 14
         },
         "text": "index=bar",
         "function": "es"
      }
   ],
   "variables": {}
}

Basically we just need to take the caret position and compare it to the location.min and location.max in each object in functions and args. That should give you an idea of if the user is typing a new function, new argument or an argument value. Once you know that you could use one of 3 suggester method to know what to pass to insertAtLocation. The only method currently implemented is for functions, but it would be trivial to add the user 2 once the caret context thing is sorted.

@rashidkpc

This comment has been minimized.

Show comment
Hide comment
@rashidkpc

rashidkpc Apr 27, 2017

Member

It looks like some other CSS changes at some point had partial broken the intended behavior of the interval select. This used to just appear as an editable drop down, it looks like some CSS changed at some point that made it look like a bad implementation of that intention.
before_other

In any case, the behavior of this pull isn't correct. This makes it look like there are 2 inputs that do 2 things. We want it to look like 1 input that can be customized if needed. You could probably use some library for this, we opted to implement it in our code to avoid pulling in another dependency.
after_other

Member

rashidkpc commented Apr 27, 2017

It looks like some other CSS changes at some point had partial broken the intended behavior of the interval select. This used to just appear as an editable drop down, it looks like some CSS changed at some point that made it look like a bad implementation of that intention.
before_other

In any case, the behavior of this pull isn't correct. This makes it look like there are 2 inputs that do 2 things. We want it to look like 1 input that can be customized if needed. You could probably use some library for this, we opted to implement it in our code to avoid pulling in another dependency.
after_other

@rashidkpc

This changes the behavior of the "other" selection in the interval chooser, it now opens a 2nd dropdown instead of making the existing dropdown appear to be editable

@cjcenizal

This comment has been minimized.

Show comment
Hide comment
@cjcenizal

cjcenizal Apr 27, 2017

Contributor

@rashidkpc Thanks for catching that. Fixed:

image

image

Contributor

cjcenizal commented Apr 27, 2017

@rashidkpc Thanks for catching that. Fixed:

image

image

@ppisljar

This comment has been minimized.

Show comment
Hide comment
@ppisljar

ppisljar Apr 28, 2017

Member

timelion

with other dropdown selection (and second field appearing) ... still doens't look completely OK ... you can see that the dropdown gets super narrow, hardly fitting the values, where before selecting other there is 'the right amount' of padding around the values.

Member

ppisljar commented Apr 28, 2017

timelion

with other dropdown selection (and second field appearing) ... still doens't look completely OK ... you can see that the dropdown gets super narrow, hardly fitting the values, where before selecting other there is 'the right amount' of padding around the values.

<timelion-interval
model="state.interval"
in-header="true"
></timelion-interval>

This comment has been minimized.

@ppisljar

ppisljar Apr 28, 2017

Member

should we /> as there is nothing inside the timelion-interval element ?

@ppisljar

ppisljar Apr 28, 2017

Member

should we /> as there is nothing inside the timelion-interval element ?

This comment has been minimized.

@cjcenizal

cjcenizal Apr 28, 2017

Contributor

Angular requires you to actually use a closing tag for custom directives.

@cjcenizal

cjcenizal Apr 28, 2017

Contributor

Angular requires you to actually use a closing tag for custom directives.

@cjcenizal

This comment has been minimized.

Show comment
Hide comment
@cjcenizal

cjcenizal Apr 28, 2017

Contributor

@ppisljar Thanks for the review! I addressed your comments. Which browser did you use when you found that select bug? I can't repro in Chrome on OS X. Would you mind checking out 3e53525 and letting me know if the select bug exists in that commit? If so, then it's a pre-existing bug and I think we should mark it in an issue and address it in another PR.

Contributor

cjcenizal commented Apr 28, 2017

@ppisljar Thanks for the review! I addressed your comments. Which browser did you use when you found that select bug? I can't repro in Chrome on OS X. Would you mind checking out 3e53525 and letting me know if the select bug exists in that commit? If so, then it's a pre-existing bug and I think we should mark it in an issue and address it in another PR.

@rashidkpc

This comment has been minimized.

Show comment
Hide comment
@rashidkpc

rashidkpc May 1, 2017

Member

Here's what I was going for:

interval2

Here's the CSS I used:

select.timelion-interval {
  width: 80px;
}

input.timelion-interval {
  width: 50px;
}

select.timelion-interval-compact {
  width: 10px;
  padding-left: 0;
  border-left: none;
}
Member

rashidkpc commented May 1, 2017

Here's what I was going for:

interval2

Here's the CSS I used:

select.timelion-interval {
  width: 80px;
}

input.timelion-interval {
  width: 50px;
}

select.timelion-interval-compact {
  width: 10px;
  padding-left: 0;
  border-left: none;
}
@cjcenizal

This comment has been minimized.

Show comment
Hide comment
@cjcenizal

cjcenizal May 2, 2017

Contributor

Thanks @rashidkpc. I tweaked your suggestion a bit and landed on this:

timelion_interval

I don't want to remove the border from the select because it makes the focus state look funky, and also makes the hit area harder to see. Could you take another look?

Contributor

cjcenizal commented May 2, 2017

Thanks @rashidkpc. I tweaked your suggestion a bit and landed on this:

timelion_interval

I don't want to remove the border from the select because it makes the focus state look funky, and also makes the hit area harder to see. Could you take another look?

@rashidkpc

This comment has been minimized.

Show comment
Hide comment
@rashidkpc

rashidkpc May 2, 2017

Member

That works for me

Member

rashidkpc commented May 2, 2017

That works for me

@cjcenizal

This comment has been minimized.

Show comment
Hide comment
@cjcenizal

cjcenizal May 2, 2017

Contributor

jenkins test this

Contributor

cjcenizal commented May 2, 2017

jenkins test this

@ppisljar

This comment has been minimized.

Show comment
Hide comment
@ppisljar

ppisljar May 3, 2017

Member

@cjcenizal the dropdown issue is not present in 3e53525 (it works ok there). it happens in latest chrome, looks better in firefox.

also another small issue i found: when you click the dropdown (in firefox) you can see the "focus border" on the input that you don't really see ....

timeion_dropdown

Member

ppisljar commented May 3, 2017

@cjcenizal the dropdown issue is not present in 3e53525 (it works ok there). it happens in latest chrome, looks better in firefox.

also another small issue i found: when you click the dropdown (in firefox) you can see the "focus border" on the input that you don't really see ....

timeion_dropdown

@cjcenizal cjcenizal merged commit 14e38cc into elastic:master May 4, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

@cjcenizal cjcenizal deleted the cjcenizal:refactor/timelion-expression-suggestions branch May 4, 2017

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request May 4, 2017

Refactor Timelion expression suggestion logic for clarity. (elastic#1…
…1285)

* Refactor Timelion expression suggestion logic for clarity.
- Create timelionExpressionInput directive.
- Create timelionExpressionSuggestions directive.
- Create expression_directive_helpers service.

cjcenizal added a commit that referenced this pull request May 4, 2017

Refactor Timelion expression suggestion logic for clarity. (#11285) (#…
…11597)

* Refactor Timelion expression suggestion logic for clarity.
- Create timelionExpressionInput directive.
- Create timelionExpressionSuggestions directive.
- Create expression_directive_helpers service.

Dreadnoth added a commit to Dreadnoth/kibana that referenced this pull request Aug 8, 2017

Refactor Timelion expression suggestion logic for clarity. (elastic#1…
…1285) (elastic#11597)

* Refactor Timelion expression suggestion logic for clarity.
- Create timelionExpressionInput directive.
- Create timelionExpressionSuggestions directive.
- Create expression_directive_helpers service.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment