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

Fix suggestions in Timelion Visualization. #11638

Conversation

Projects
None yet
3 participants
@cjcenizal
Copy link
Contributor

cjcenizal commented May 6, 2017

This fixes a bug introduced by #11285. When you create a Timelion visualization, suggestions weren't showing up. This PR fixes that, so now they show up again. Also, the suggestions were no longer clickable. This PR also fixes that.

As part of this solution, I refactored timelionExpressionInput to use ng-transclude to accept any type of form control.

Refactor timelionExpressionInput to use ng-transclude to accept any t…
…ype of form control. Fix suggestions in Timelion Visualization.

@cjcenizal cjcenizal force-pushed the cjcenizal:bug/timelion-visualization-expression-suggestions branch from a82f8a2 to c0d2808 May 6, 2017

@cjcenizal cjcenizal requested a review from rashidkpc May 6, 2017


$scope.functionSuggestions.reset();
scope.functionSuggestions.reset();

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal May 9, 2017

Author Contributor

Beyond this point I pretty much just renamed $scope to scope, since the $ is used to indicate a service injected via Angular's DI, but the scope here is just a regular function parameter.

This comment has been minimized.

Copy link
@ppisljar

ppisljar May 10, 2017

Member

would be nice if this would be separate commit, would make it easier to review

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal May 10, 2017

Author Contributor

Thanks man, you're right. I should have split it out. 😭

This comment has been minimized.

Copy link
@ppisljar

ppisljar May 10, 2017

Member

no worries :)

ng-model="sheet"
timelion-expression="{{sheet}}"
placeholder="Expression..."
aria-label="Expression input"

This comment has been minimized.

Copy link
@ppisljar

ppisljar May 10, 2017

Member

weren't we suppose to have area-labels everywhere due to accessibility ?

This comment has been minimized.

@@ -9,6 +9,6 @@
ng-show="functionSuggestions.isVisible"
suggestions="functionSuggestions.list"
selected-index="functionSuggestions.index"
on-click-suggestion="suggestionClickHandler"

This comment has been minimized.

Copy link
@ppisljar

ppisljar May 10, 2017

Member

where did this handler go ? can we remove it ?

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal May 10, 2017

Author Contributor

I think I added it to the template in my refactoring PR (https://github.com/elastic/kibana/pull/11285/files#diff-6bf4e46df33f0b22d200f253c6ce536cR21), but forgot to add the corresponding method in the directive.

@@ -5,7 +5,7 @@
<div
class="suggestion"
data-suggestion-list-item
ng-click="completeExpression($index)"
ng-click="onClickSuggestion({ suggestionIndex: $index })"

This comment has been minimized.

Copy link
@ppisljar

ppisljar May 10, 2017

Member

why ? this calls onClickSuggestion which just calls completeExpression ...
i don't see any benefits, but additional code to read and nested function call ?

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal May 10, 2017

Author Contributor

This is a separate directive, so it accepts a handler function via an attribute which gets called when it's clicked. It's analogous to how React components accept callbacks as props.

@@ -47,8 +47,9 @@ app.directive('timelionExpressionInput', function ($compile, $http, $timeout) {
sheet: '=',
},
replace: true,
transclude: true,

This comment has been minimized.

Copy link
@ppisljar

ppisljar May 10, 2017

Member

is this fixing the original bug or should this be a separate PR ?

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal May 10, 2017

Author Contributor

This is fixing the original bug. It allows us to inject any kind of input element (e.g. <input> or <textarea) into this directive, and provide suggestions based on the user's input. In my multiline expression PR, I'll inject a <div contenteditable="true"> into it.

@ppisljar

This comment has been minimized.

Copy link
Member

ppisljar commented May 10, 2017

the bug fix works (so the suggestions do show and i can click them), added some minor comments ...

and some more things i noticed, which are not related to this PR:

some general autosuggestion isues:

  • when writing a function name (lets say .value) documentation for the parameters shows up, but as soon as you hit ( they dissapear ... which is just at the time that i would actually need them. So i end up removing that ( and learning parameters by mind and then continuing ... which is far from a nice workflow.

some issues with the multiline editor (not sure if related):

  • in timelion (from menu, not from visualize) there is no multiline editor .... (not sure if related to this PR, but there should be as well)
  • suggestions stop working after i hit a new line
  • suggestions show below the text area ... and are easy to miss (if your textarea is the size of your page you will not see suggestions as they will be added to the bottom)
@ppisljar
Copy link
Member

ppisljar left a comment

LGTM

@cjcenizal cjcenizal merged commit f56736f into elastic:master May 10, 2017

2 checks passed

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

@cjcenizal cjcenizal deleted the cjcenizal:bug/timelion-visualization-expression-suggestions branch May 10, 2017

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

Fix suggestions in Timelion Visualization. (elastic#11638)
* Refactor timelionExpressionInput to use ng-transclude to accept any type of form control. Fix suggestions in Timelion Visualization.
* Fix bug so that users can click on suggestions to select them.
* Fix broken reference to input element.

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

Fix suggestions in Timelion Visualization. (#11638) (#11699)
* Refactor timelionExpressionInput to use ng-transclude to accept any type of form control. Fix suggestions in Timelion Visualization.
* Fix bug so that users can click on suggestions to select them.
* Fix broken reference to input element.

snide added a commit to snide/kibana that referenced this pull request May 30, 2017

Fix suggestions in Timelion Visualization. (elastic#11638)
* Refactor timelionExpressionInput to use ng-transclude to accept any type of form control. Fix suggestions in Timelion Visualization.
* Fix bug so that users can click on suggestions to select them.
* Fix broken reference to input element.

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

Fix suggestions in Timelion Visualization. (elastic#11638) (elastic#1…
…1699)

* Refactor timelionExpressionInput to use ng-transclude to accept any type of form control. Fix suggestions in Timelion Visualization.
* Fix bug so that users can click on suggestions to select them.
* Fix broken reference to input element.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.