Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

feat(autocomplete): Add promise support to md-item-text #2710

Closed
wants to merge 2 commits into from

Conversation

epelc
Copy link
Contributor

@epelc epelc commented May 4, 2015

This adds promise support for autocompletes md-item-text option. It makes use of $q.when() in getDisplayValue() to convert all functions to promises. Because of this I had to change several things around to make the rest of the autocomplete controller work with promises when it used getDisplayValue().

I tried running the karma tests and got a few errors related to md-autocomplete but I also got errors when I ran it on the just released 0.9.0. I checked several things on the docs auto complete demo locally with this build and also in my app and it functions the same as the official docs so I don't think I broke anything.

This should close #2462 If you want to see a use case see that issue I explained how it's needed for the google places api. But in general this is needed anytime you need to fill in extra data after you select something.

@ThomasBurleson Can you take a look at this please?

Review on Reviewable

@ThomasBurleson ThomasBurleson added this to the 0.10.0 milestone May 4, 2015
@ThomasBurleson ThomasBurleson self-assigned this May 4, 2015
@gkalpak
Copy link
Member

gkalpak commented May 7, 2015

Without giving it too much thought, I suspect that this will lead to unexpected results if a promise gets resolved after the next call to getDisplayValue().

@epelc
Copy link
Contributor Author

epelc commented May 7, 2015

@gkalpak I think your right.

$q doesn't have a cancel() so I think we could possibly assign the promises id's and check if its the latest when we go to use it. But I'm open to any other suggestions. I think this would really only emerge when a user has a bad network but it's still a problem.

I made select() clear the search text and selected item when getDisplayValue() is rejected.

@epelc
Copy link
Contributor Author

epelc commented Jun 3, 2015

@ThomasBurleson Any chance this will be merged for 0.10.0? I can get it working 0.9.7 today if you want to try and merge it. I've had about 20 people using it for a while now.

@ThomasBurleson
Copy link
Contributor

@epelc - working on this now.
@robertmesserle - can you review this; since your are the primary author of AutoComplete.

@epelc
Copy link
Contributor Author

epelc commented Jul 1, 2015

@ThomasBurleson Cool! Do you want me to get it working with the current master? There were several additions and fixes for auto complete since I first created this but it shouldn't take too long to update.

@robertmesserle
Copy link
Contributor

@ThomasBurleson @epelc 👍

@ThomasBurleson
Copy link
Contributor

@epelc - yes. plz rebase with master.

@epelc epelc force-pushed the autocomplete-itemText-promise branch from 727b1c1 to 861c426 Compare July 2, 2015 03:39
@epelc
Copy link
Contributor Author

epelc commented Jul 2, 2015

Everything works besides the tests now. I'll get them running first thing tomorrow morning. I was having trouble getting karma to run on this machine.

Also I found #3546 while upgrading everything.

@epelc epelc force-pushed the autocomplete-itemText-promise branch from 861c426 to a7151b2 Compare July 2, 2015 13:30
@epelc
Copy link
Contributor Author

epelc commented Jul 2, 2015

@robertmesserle Can you take a quick look?

@ThomasBurleson
Copy link
Contributor

@epelc - you did not provide a unit test for this promise-based internal feature.
Currently we have:

/**
   * Returns the display value for an item.
   * @param item
   * @returns {*}
   */
  function getDisplayValue (item) {
    return $q.when( getItemText(item) || item );

    /**
     * Getter function to invoke user-defined expression (in the directive)
     * to convert your object to a single string.
     */
    function getItemText(item) {
      return (item && $scope.itemText) ? $scope.itemText(getItemAsNameVal(item)) : null;
    }
  }

where the $scope.itemText() expression may return a promise. We should test that....

I will merge this PR. Can you provide another with unit tests?

@epelc
Copy link
Contributor Author

epelc commented Jul 2, 2015

@ThomasBurleson I'm trying to make a unit test for it now. I can just add it to this if you want.

@ThomasBurleson
Copy link
Contributor

Oops.

@epelc
Copy link
Contributor Author

epelc commented Jul 2, 2015

It's fine I'll make a new pr.

Does this look ok? I haven't written many karma tests before. I have this right after the basic functionality tests here. Would you organize it this way or make a new Describe()?

it('should support promises with md-item-text', inject(function($timeout, $mdConstant, $q) {
      var scope = createScope();

      scope.displayItem = function(item) {
        var deferred = $q.defer();

        $timeout(function() {
          deferred.resolve(item.display+'-promise');
        });

        return deferred.promise;
      }

      var template = '\
          <md-autocomplete\
              md-selected-item="selectedItem"\
              md-search-text="searchText"\
              md-items="item in match(searchText)"\
              md-item-text="displayItem(item)"\
              placeholder="placeholder">\
            <span md-highlight-text="searchText">{{item.display}}</span>\
          </md-autocomplete>';
      var element = compile(template, scope);
      var ctrl    = element.controller('mdAutocomplete');

      element.scope().searchText = 'fo';
      element.scope().$apply();
      $timeout.flush();

      ctrl.keydown({ keyCode: $mdConstant.KEY_CODE.DOWN_ARROW, preventDefault: angular.noop });
      ctrl.keydown({ keyCode: $mdConstant.KEY_CODE.ENTER, preventDefault: angular.noop });
      scope.$apply();
      $timeout.flush();

      expect(scope.searchText).toBe('foo-promise');
    }));

@epelc
Copy link
Contributor Author

epelc commented Jul 2, 2015

@ThomasBurleson ping

Can you look at the above. I forgot to mention you.

Splaktar pushed a commit that referenced this pull request Jul 4, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

md-autocomplete: Support md-item-text evals as promises
4 participants