fix(#271) if option is set the refresh function is getting the request. #1845

Jefiozie · 2016-11-03T14:51:59Z

I think this is something that is requested multiple times: #271, #625,#619.

Used the initial changes from 625 with some minor adjustments to let everything work and only watch variables if needed.

If this is a viable solution i think all the other can be closed down.

…ction is getting the request.

user378230 · 2016-11-03T15:17:37Z

Could we instead pass ctrl.open as an argument to the refresh function?

Jefiozie · 2016-11-03T15:27:37Z

Instead of the refreshIsActive ? Will run some tests with it.

DId some test changing the isActive to open will have the same behaviour.

user378230 · 2016-11-03T16:05:40Z

Ah we actually have two "issues" here:

refresh is being called when ui-select is initialized (but not opened)

refresh is not called when select is activated

Can be solved by passing ctrl.open as an argument to the refresh function (inside the refresh function users can say if select is not open then don't call server)
Maybe actually user's can already do this themselves, by defining refresh="fetchFromServer($select.search, $select.open)"?
Can be solved by calling refresh in $select.open $watch like you did.

For 2 we don't need a new attribute, just call refresh every time, as we call refresh whenever $select.search changes anyway.

Jefiozie · 2016-11-04T09:10:14Z

But if we look if ctrl.open is true we could fix also point 1 right? If you search but it is not active (open will be set to true in the function active). This will prevent the refresh function to go off on initializing and only execute the function when it is active. Right?

Jefiozie · 2016-11-04T09:55:00Z

Did some test to verify if this solution is working.....and it will break a lot of tests. Need to dig a little bit deeper because in the test the function openDropDown will set the ctrl.open manually to true. I thought it should be $select.activate() but still problems. Ideas are welcome 😉

user378230 · 2016-11-05T14:41:17Z

@Jefiozie I think you misunderstand, we do not need to do anything with ctrl.open because user can already do refresh="fetchFromServer($select.search, $select.open)" right?

Then the user can check manually in fetchFromServer(...) if select is open.

I cannot test this right now but I think it should be correct?

The only thing extra we need to do it, is call ctrl.refresh in activate (so that refresh is called whenever select is opened).

Does that make sense?

user378230 · 2016-11-05T14:43:57Z

Tagging #1174 for reference.

Jefiozie · 2016-11-06T14:51:30Z

@user378230 , understand you now and agree on both points. Will remove the extra parameters/ variables and add the ctrl.refresh in activate function

Jefiozie · 2016-11-06T16:17:21Z

@user378230 ,
So making these changes have a lot of impact. This peace of code is executed on initializing uiSelectChoicesDirective.js#L68. Adding a check on this if the $select.open equals true (as this is set by the activate function) it will break all tests but has the behavior as it should ( I believe).

Next problem:
In a use case where the ui-select doesn't use the refresh functionality it will not set the items until it has a search value. Trying to find out why this is happening, if you don't use the refresh it should just set the items. But it looks like the have a relation somewhere.

Example of this problem

Keep you posted

user378230 · 2016-11-07T10:48:13Z

src/uiSelectChoicesDirective.js

@@ -55,10 +55,9 @@ uis.directive('uiSelectChoices',


        $select.parseRepeatAttr(attrs.repeat, groupByExp, groupFilterExp); //Result ready at $select.parserResult
-
+        $select.refreshOnActive = scope.$eval(attrs.refreshOnActive);


Lets remove the attribute, this can be default functionality. We just call refresh more often now. (ie. When search.length == 0)

user378230 · 2016-11-07T10:49:06Z

src/uiSelectChoicesDirective.js

@@ -68,7 +67,7 @@ uis.directive('uiSelectChoices',
        scope.$watch('$select.search', function(newValue) {
          if(newValue && !$select.open && $select.multiple) $select.activate(false, true);
          $select.activeIndex = $select.tagging.isActivated ? -1 : 0;
-          if (!attrs.minimumInputLength || $select.search.length >= attrs.minimumInputLength) {
+          if ((!attrs.minimumInputLength || $select.search.length >= attrs.minimumInputLength) && (!$select.refreshOnActive || ($select.refreshOnActive && $select.open))) {


We can undo changes here also. It will be up to user to check in their own function.

user378230 · 2016-11-07T10:52:36Z

src/uiSelectChoicesDirective.js

        scope.$watch('$select.open', function(open) {
          if (open) {
            tElement.attr('role', 'listbox');
+            if(!angular.isUndefined($select.refreshOnActive)){


I wonder instead if we do this in a uis:activate listener? (So it's clearer)

Jefiozie · 2016-11-07T11:55:40Z

@user378230 I removed this already only I didn't commit it. Can commit the latest changes if you want?
All test will fail though.

user378230 · 2016-11-07T12:30:04Z

Commit what you have I can always pull down and test myself 😁

Jefiozie · 2016-11-07T12:53:21Z

Here it is 😈

user378230 · 2016-11-07T13:34:09Z

src/uiSelectChoicesDirective.js

@@ -68,7 +66,7 @@ uis.directive('uiSelectChoices',
        scope.$watch('$select.search', function(newValue) {
          if(newValue && !$select.open && $select.multiple) $select.activate(false, true);
          $select.activeIndex = $select.tagging.isActivated ? -1 : 0;
-          if (!attrs.minimumInputLength || $select.search.length >= attrs.minimumInputLength) {
+          if ((!attrs.minimumInputLength || $select.search.length >= attrs.minimumInputLength) && $select.open) {


How come you changed this? I didn't think it would be needed? Does reverting this fix tests?

I changed this as this will prevent the initial request of the refresh function on initializing.
Yes reverting will fix the tests.

So we can revert this then. 😁

From my comment a few days ago:

the user can check manually in fetchFromServer(...) if select is open

Based on your comment yes we can revert. But still on initializing the refresh will be exectued. And this is not the behavior as it should be i think and also the problem of #271 ?

As I said before I want the user to check if it's open not us, users should already be able to do this. (I'm writing on mobile so my replies are short sorry!).

I also don't want to add breaking change as some users might like to load on initialise.

Okay, if that is the case then we can revert almost everything. Only thing we need to add is when activating/focus the control we need to execute the refreshfunction.

The users can already check if it is open by adding (as you already mentioned) a second param $select.open

Yup 😁 if I could add functionality while reducing lines of code I would! 😛 Hopefully we both learned something more about ui-select though 😂

user378230 · 2016-11-07T13:34:54Z

src/uiSelectController.js

@@ -257,8 +254,6 @@ uis.controller('uiSelectCtrl',
      if (ctrl.dropdownPosition === 'auto' || ctrl.dropdownPosition === 'up'){
        $scope.calculateDropdownPos();
      }
-
-      $scope.$broadcast('uis:refresh');


Why was the removed?

Because nothing is listing on it.

Can't users listen on it? 🤔 I don't use events much in angular so not completely sure!

Yes you could both I cannot think of any use case where you need this? And if we want to support this it should be in the refreshfunction instead of activate.

No harm is keeping it there now, someone might use it already and we should avoid breaking their use case 😬

agree, but still it doesn't feel good to leave code that does nothing.

Jefiozie · 2016-11-07T19:01:24Z

@user378230, so reverted the broadcast event. But also found a other use case where we could fix the original problem.By checking if the newValue !== '' and check ifnewValue !== oldValue we will prevent the initial call on load. As the watch is fired every time a user types in a char it will execute the refresh function.

Second we will fire a refresh on activating the ui-select where the value of $select.search is already empty (as it should because nothing is present at that time).

Do you believe this is a good way to solve the problem?

p.s. all test are passing. 😄

user378230 · 2016-11-08T08:30:45Z

But also found a other use case where we could fix the original problem.By checking if the newValue !== '' and check ifnewValue !== oldValue we will prevent the initial call on load. As the watch is fired every time a user types in a char it will execute the refresh function.

Isn't this the use case I said we wanted to keep? (Calling the refresh function when the directive is first loaded?) Or am I misunderstanding what you said...

After this PR we should have 3 different routes to calling refresh:

When directive is first loaded ($select.search = "", $select.open = false) - existing
When ui-select is opened ($select.search = "", $select.open = true) - new
When $select.search changes ($select.search = "xyz", $select.open = true) - existing

You are making me work hard for this PR haha! 😆

Jefiozie · 2016-11-08T10:11:13Z

@user378230 , thanks for all the feedback great PR 😆!
Then this is it (I believe) reverted all changed and added the new use case.

When ui-select is opened ($select.search = "", $select.open = true) - new

With some tests of course.

user378230 · 2016-11-08T13:22:42Z

@Jefiozie Think you just need to review your commit once more (can you revert empty line changes too and an extra whitespace)

And tests are like gold dust in library, so the more the better! 👍

Jefiozie · 2016-11-08T20:10:35Z

@user378230, so after this intensive PR 😈 i think we have learned and made it a little beter. Right 😉 , again thanks for the feedback/ help and good discussion.

user378230 · 2016-11-09T08:08:57Z

@Jefiozie check the files tab on here please... 🙂

This reverts commit d7a5df2.

Jefiozie · 2016-11-09T18:34:22Z

@user378230 ,Think it looks better now.

user378230 · 2016-11-15T16:24:57Z

Queued for next released. Just want to undo the unnecessary empty line changes before I merge.

Jefiozie · 2016-12-04T19:48:33Z

@user378230 , Any ideas when this will be merged?

aaronroberson · 2017-01-04T16:34:13Z

@Jefiozie Can you resolve the conflicts so that I can merge? Thanks for all your work!

Jefiozie · 2017-01-05T06:41:25Z

Sure will have a look.

Does @user378230 not maintain this repository any more or is he gone (as far as you know)?

Jefiozie · 2017-01-13T07:31:50Z

@aaronroberson , see my previous comment.

Does @user378230 not maintain this repository any more or is he gone (as far as you know)?
Many issue and PR's are still open and I think we need to do something about it 😄

Jefiozie · 2017-01-16T12:37:16Z

@aaronroberson , made the changes. I can be merged also can you reply on previous comments?

Jefiozie · 2017-01-19T17:58:18Z

@aaronroberson sorry to bother you but I'm seeing the issue list grow and want to help getting this a little bit smaller. Can you merge this? Thanks.

Jefiozie · 2017-01-22T18:46:01Z

@aaronroberson @user378230 sorry to bother you guys. But this libary needs some attention.
Can I maybe help you guys in a way?

CC @wesleycho , maybe you can contact people above? (thanks and sorry to bother you just wanne keep te libary "up to date")

aaronroberson · 2017-01-23T16:47:03Z

Can you send me an email to discuss having you help maintain the project? My email is aaronaroberson at gmail.

fix(angular-ui#271) if option is set the refresh function is getting the request.

houwei544 · 2019-03-27T01:59:34Z

when ui-select is multiple , function refresh can't get $select .

fix(angular-ui#271) if option is set only on activate the refresh fun…

84420c3

…ction is getting the request.

Instead of new var used the open variable as it is set on activate.

d137ded

Jefiozie closed this Nov 4, 2016

Jefiozie reopened this Nov 4, 2016

user378230 reviewed Nov 7, 2016

View reviewed changes

Changed so that attr is removed and watch is checkin select.open

57f612a

user378230 reviewed Nov 7, 2016

View reviewed changes

Jefiozie added 2 commits November 7, 2016 19:50

Reverted: removed broadcast event

91e48dd

changed watch of search as the initial value should be ignored on load

2a8509f

Jefiozie added 2 commits November 8, 2016 11:08

reverted the watch function for the desired use cases.

a52f0b2

Added test for initial refresh call

d7a5df2

removed some whitespace

98c179a

Jefiozie added 2 commits November 9, 2016 19:16

Revert "Added test for initial refresh call"

7531d29

This reverts commit d7a5df2.

removed empty line

119b072

Merge branch 'master' into 625_refreshOnActive

914f2d8

Jefiozie merged commit 3642a1d into angular-ui:master Jan 25, 2017

This was referenced Jan 25, 2017

refresh on active #625

Closed

Need ability to postpone refresh function call until select is open #619

Closed

Lazy options loading! #1174

Open

cloudrick pushed a commit to hardcoretech/ui-select that referenced this pull request Feb 17, 2017

Merge pull request angular-ui#1845 from Jefiozie/625_refreshOnActive

92d0eae

fix(angular-ui#271) if option is set the refresh function is getting the request.

Jefiozie mentioned this pull request Mar 15, 2017

Wait for the first open to first load remote data #271

Closed

Jefiozie deleted the 625_refreshOnActive branch March 21, 2017 07:12

aske-cph mentioned this pull request May 15, 2019

Disable first refresh call #1197

Open

		@@ -55,10 +55,9 @@ uis.directive('uiSelectChoices',


		$select.parseRepeatAttr(attrs.repeat, groupByExp, groupFilterExp); //Result ready at $select.parserResult

		$select.refreshOnActive = scope.$eval(attrs.refreshOnActive);

fix(#271) if option is set the refresh function is getting the request. #1845

fix(#271) if option is set the refresh function is getting the request. #1845

Conversation

Jefiozie commented Nov 3, 2016 • edited Loading

user378230 commented Nov 3, 2016

Jefiozie commented Nov 3, 2016 • edited Loading

user378230 commented Nov 3, 2016 • edited Loading

Jefiozie commented Nov 4, 2016

Jefiozie commented Nov 4, 2016

user378230 commented Nov 5, 2016 • edited Loading

user378230 commented Nov 5, 2016

Jefiozie commented Nov 6, 2016

Jefiozie commented Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

user378230 Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Jefiozie commented Nov 7, 2016 • edited Loading

user378230 commented Nov 7, 2016

Jefiozie commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefiozie Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefiozie Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Jefiozie commented Nov 7, 2016

user378230 commented Nov 8, 2016 • edited Loading

Jefiozie commented Nov 8, 2016 • edited Loading

user378230 commented Nov 8, 2016

Jefiozie commented Nov 8, 2016

user378230 commented Nov 9, 2016

Jefiozie commented Nov 9, 2016 • edited Loading

user378230 commented Nov 15, 2016

Jefiozie commented Dec 4, 2016

aaronroberson commented Jan 4, 2017

Jefiozie commented Jan 5, 2017 • edited Loading

Jefiozie commented Jan 13, 2017

Jefiozie commented Jan 16, 2017

Jefiozie commented Jan 19, 2017

Jefiozie commented Jan 22, 2017

aaronroberson commented Jan 23, 2017

houwei544 commented Mar 27, 2019

Jefiozie commented Nov 3, 2016 •

edited

Loading

Jefiozie commented Nov 3, 2016 •

edited

Loading

user378230 commented Nov 3, 2016 •

edited

Loading

user378230 commented Nov 5, 2016 •

edited

Loading

Jefiozie commented Nov 6, 2016 •

edited

Loading

user378230 Nov 7, 2016 •

edited

Loading

Jefiozie commented Nov 7, 2016 •

edited

Loading

Jefiozie Nov 7, 2016 •

edited

Loading

Jefiozie Nov 7, 2016 •

edited

Loading

user378230 commented Nov 8, 2016 •

edited

Loading

Jefiozie commented Nov 8, 2016 •

edited

Loading

Jefiozie commented Nov 9, 2016 •

edited

Loading

Jefiozie commented Jan 5, 2017 •

edited

Loading