search

vitalets / angular-xeditable

Edit in place for AngularJS
http://vitalets.github.io/angular-xeditable
MIT License
1.91k stars 403 forks source link

added ability to add more then one editable-ui-select component on the same page. #464

Closed jkrikunenko closed 8 years ago

jkrikunenko commented 8 years ago

updated directives/uiselect.js

ckosloski commented 8 years ago

You should only check in your changes to src/js/directives/uiselect.js, none of the dist files should be checked in.

Also, are all of the tests still working with this change?

jkrikunenko commented 8 years ago

@ckosloski Thanks, I've updated pull request, delete unnecessary files. And yes, all tests are passed.

jkrikunenko commented 8 years ago

@eugef done. Please, take a look.

eugef commented 8 years ago

@ckosloski, could you please check if this PR doesn't break existing fucntionality. Is it really impossible without this PR to have several ui-select on thesame page?

ckosloski commented 8 years ago

The tests do all run and everything seems to work OK on the demo page. Maybe a new dev test with multiple ui-selects on the page should be added?

I'm not sure if it's impossible or not to have multiple ui-selects on the page without the PR. I would think it would work OK the way it is since the directive is searching for the sub-elements within the element the directive is placed on.

@julia-k can you explain some more what the issue is you are having when you are using mulitple ui-selects?

ckosloski commented 8 years ago

I started working on a new dev-test for ui-select with two ui-select's on one form and they both appear to work normally. I did have to upgrade to the latest version of ui-select (0.16.1).

eugef commented 8 years ago

@ckosloski, do you mean that multiple ui-selects work normally even without PR?

ckosloski commented 8 years ago

Yes, at least in the test case I was working on (one editable form with two ui-selects that are active). We need to find out what use case this fix is for.

eugef commented 8 years ago

Ok, let's wait for the answer of @julia-k.

eugef commented 8 years ago

Tests are added by #467. As i can see this bug is not confirmed and having multiple ui-selects on the same page is not an issue.

I am closing this PR for now. @julia-k if you can provide code example that reproduce the issue, please reopen this ticket.

ckosloski commented 8 years ago

@eugef I think this issue needs to be reopened and merged. I was not testing the right scenario with this. If you look at #481, there are two inputs with different sets of data. I did verify locally that these changes fix the issues in 481.

TomOffringa commented 8 years ago

Hi,

Thanks for referencing this error @ckosloski! I looked through the issues for something similar for my issue https://github.com/vitalets/angular-xeditable/issues/481 but I guess I oversaw this one.

The changes in this PR do indeed fix my issues, so if this PR doesn't break anything else, this would be a good solution.

eugef commented 8 years ago

@ckosloski, @TomOffringa, PR is merged

NalinSajwan commented 8 years ago

Hi, when will this be available in a release?

ckosloski commented 8 years ago

@eugef could you do a release?

eugef commented 8 years ago

Will do a release tomorrow