indrimuska / angular-selector

A native AngularJS directive that transform a simple <select> box into a full html select with typeahead.
http://indrimuska.github.io/angular-selector
MIT License
96 stars 36 forks source link

Fix to highlight selected item #55

Closed shanmugam-siva closed 8 years ago

shanmugam-siva commented 8 years ago

Could you please review and let me know if any changes required?

indrimuska commented 8 years ago

Hi @shanmugam-siva, I suppose this refers to #54. The main differences between the master branch and your PR are this (#1) and this (#2).

Let's talk about those changes:

  1. Every time selectedValues updates itself, only if there's no multiple select configuration, you update highlighted index to the index of the selected newValue element in the options array. Ok, this looks nice, but you should always use scope.highlight() method, in order to prevent overflow errors, instead of directly updating scope.highlighted index. Also, it's better to scroll the dropdown menu to the highlighted element every time the selector is opened. This could be easily done by updating scope.open method like this:

    scope.open = function () {
       scope.isOpen = true;
       scope.dropdownPosition();
       $timeout(scope.scrollToHighlighted); // <--- add this line
    };
  2. I think that if we correctly use scope.highlight function we will never have a negative highlighted index, so maybe this section can be removed. However, we can move the change nr. 1 at this point, like this:

    scope.filterOptions = function () {
       scope.filteredOptions = filter(scope.options || [], scope.search);
       if (scope.multiple)
           scope.filteredOptions = scope.filteredOptions.filter(function (option) {
               var selectedValues = angular.isArray(scope.selectedValues) ? scope.selectedValues : [scope.selectedValues];
               return !scope.inOptions(selectedValues, option);
           });
       else                                                                            // <-- add these
           scope.highlight(scope.filteredOptions.indexOf(scope.selectedValues[0]));    // <-- two lines
    };

What are your thoughts about that?

PS: please, follow the standard coding style before pushing your next commit using grunt eslint. Thank you very much for your time. :)

shanmugam-siva commented 8 years ago

Hi @indrimuska . Thanks for your time. I will raise new pull request with the suggested changes

indrimuska commented 8 years ago

👍 thanks to you man!