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

Autocomplete refactor #283

Merged
merged 5 commits into from
Mar 10, 2016

Conversation

canufeel
Copy link
Contributor

This is refactored version of #199 which adds:

  • fullTextSearch support for autocomplete
  • assigning of Ember.ObjectProxy objects to autocomplete model attribute on init or during usage.
  • Has dummy site docs updated to explain how these features work as well as some basic tests for fullTextSearch.
  • Fixes Ember.String.fmt deprecation by substituting it with Ember.String.loc
  • Adds its 5 cents to the autocomplete code readability.

@miguelcobain
Copy link
Owner

The original autocomplete (from angular material) doesn't support full-text search?

@brendanoh
Copy link

My thoughts:

@canufeel this PR looks good to me. @miguelcobain I am not sure I understand why this project is always limited by what angular material has or doesn't have.

Personally, I have used Ember Paper twice on projects and both times have had to add in full text search to "fix" the autocomplete control. Fix in quotes cause it is not a bug but definitely feels missing to me.

I mean it boils down to these lines of text:

+      if (get(this,'fullTextSearch')){
 +        return search.indexOf(searchText) !== -1;  
 +      } else {
 +        return search.indexOf(searchText) === 0;
 +      }

I get trying to emulate the work done there to be consistent but this feels like a mistake on their part that we are replicating.

@miguelcobain
Copy link
Owner

I am not sure I understand why this project is always limited by what angular material has or doesn't have.

@brendanoh this is not true. The only thing we are somewhat limited is to angular material's styles. ember-paper can have the api it wants, and the functionality it wants, as long as it doesn't clash with angular-material's styles. This isn't certainly the case.

I merely asked because it felt impossible that angular material didn't have this. I wasn't suggesting at all that we shouldn't have this. On the contrary, I think this is something very fundamental.

Just trying to understand why we don't have this in the first place.

@brendanoh
Copy link

Ahhhhhh @miguelcobain that makes so much more sense now...

In Angular Material you write your own comparator which would allow you do it any way you please.

CodePen of rewritten createFilterFor: http://codepen.io/anon/pen/adgPgq

    function createFilterFor(query) {
      var lowercaseQuery = angular.lowercase(query);
      return function filterFn(state) {
        //return (state.value.indexOf(lowercaseQuery) === 0);
        return (state.value.indexOf(lowercaseQuery) !== -1);
      };
    }

Also, now that I think about it every other time I felt limited was due to Angular styles... Thanks!

@miguelcobain
Copy link
Owner

@canufeel can you add this to the changelog so I can merge this?

@canufeel
Copy link
Contributor Author

@miguelcobain yeah, sure.

@brendanoh
Copy link

Any idea when this will be merged? I see 2 other autocomplete related PR's as well.

@canufeel
Copy link
Contributor Author

@miguelcobain Please suggest if any other changes should be made.

miguelcobain added a commit that referenced this pull request Mar 10, 2016
@miguelcobain miguelcobain merged commit d2f9229 into miguelcobain:master Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants