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

reworked the save object finder to make it more angular #4222

Merged
merged 7 commits into from
Jul 15, 2015

Conversation

BigFunger
Copy link
Contributor

  • Removed most of the jquery code and made it more angular
  • Moved $scope variables into a controller object
  • Re-enabled the keyboard navigation

@w33ble w33ble removed their assignment Jun 12, 2015
@w33ble w33ble removed the review label Jun 12, 2015
@spalger spalger self-assigned this Jun 12, 2015
@w33ble
Copy link
Contributor

w33ble commented Jun 12, 2015

Your use of self is a bit aggressive. You should only need to use that when you need the outside this context within a handler function. Here, that looks primarily limited to your $timeout calls.

type="text">
<span class="finder-hit-count"><strong>{{hitCount}}</strong> {{type}}</span>
<input input-focus
ng-model="filter"
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't do alignment like this. Just make it 2 spaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

the multi-line-ness is appreciated but lets keep it two-space indented throughout this template

@w33ble
Copy link
Contributor

w33ble commented Jun 12, 2015

The focus stuff still seems wrong. I thought if I hit tab, select something, and then hit tab again it's supposed to be on the item I originally selected. Is that not right?

2015-06-12 14_27_03

What's happening there: Tab -> arrow around -> Tab again (first anchor is now focused)

//key handler for the filter text box
self.filterKeyDown = function ($event) {
if (keymap[$event.keyCode] !== 'tab')
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curly braces are required if you're doing multi-line conditionals. Here, return should just be on the same line.

@@ -67,20 +77,22 @@ define(function (require) {
if (!$scope.userOnChoose) {
return hit.url;
}

return '#';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is causing the entire page to reload when searches are added to the dashboard. What is the '#' supposed to accomplish?

pr:
2015-06-15 19_17_56

master:
2015-06-15 19_26_47

@spalger spalger assigned BigFunger and unassigned spalger Jun 16, 2015
@BigFunger BigFunger assigned spalger and unassigned BigFunger Jul 14, 2015
spalger added a commit that referenced this pull request Jul 15, 2015
reworked the save object finder to make it more angular
@spalger spalger merged commit 5031196 into elastic:master Jul 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants