ghiscoding / Aurelia-Bootstrap-Plugins

Aurelia-Bootstrap-Plugins are Custom Elements to bridge with a set of 3rd party Bootstrap addons
https://ghiscoding.github.io/Aurelia-Bootstrap-Plugins
MIT License
45 stars 23 forks source link

abp-select selected-value binding not updated before onChanged handler exection #35

Closed Mobe91 closed 6 years ago

Mobe91 commented 6 years ago

I am using an abp-select like this:

<abp-select element.bind="picker" selected-value.bind="mySelectedValue"></abp-select>

In my controller, I attach a handler to the onChanged event like this:

export class Controller {
  @observable() mySelectedValue;
  @observable() picker;

  pickerChanged() {
    this.picker.events.onChanged = (e) => console.log(this.mySelectedValue); // this.mySelectedValue has the old value at this point
  }

  mySelectedValueChanged() {
    // this is called after the onChanged handler and the value of this.mySelectedValue is up-to-date at this point
  }
}

When I select an item from the select menu, the onChanged handler is called but this.mySelectedValue is not set to the correct value. The binding is only refreshed after the invocation of the onChanged handler.

First of all, the user supplied onChanged event handler is called before the handler registered in watchOnChangedToUpdateValueAndItemObjects which is wrong I think. But even reversing the order of change handler invocation does not fix the issue.

I think this has something to do with updating the selectedValue field inside the change event handler here. The binding update task is queued at this point and seems to be executed only after all change handlers have finished.

ghiscoding commented 6 years ago

hmm the line 459 might be better being a local variable since on line 466 there's another update applied to the selectedValue. If you have the library code, you could try change line 459 to a local variable (459: let selectedValue = this.domElm.selectpicker('val');) and use the local variable on line 460, the 2nd argument (let selection = this.findItems(this.collection, selectedValue, this.objectKey); Anyhow, I think at the end I still need to update the selectedValue and selectedItem to get both the index and it's value (that is mostly when using an array of objects instead of strings).

If you think that there's something that could be change, I certainly welcome PRs, thanks

Mobe91 commented 6 years ago

I tried to change this to a local variable but that does not address the core issue here. The binding update and the change event handler invocation happens in the wrong order. Maybe your approach of updating the bindings within your internal change handler is invalid but I am not sure about that.

ghiscoding commented 6 years ago

I haven't use it the way you mentioned, so if you find the best way then please make a PR. Or provide some code to replicate the issue, I don't visually see everything you mentioned.

Mobe91 commented 6 years ago

I have adapted your client-cli demo to show the issue by adding a 'OnChanged test' abp-select to the bootstrap-plugins view. When you select a value, the change listener logs the current binding value to the console. You will see that it is always one step behind. Aurelia-Bootstrap-Plugins.zip

ghiscoding commented 6 years ago

So I looked at your attached example and yeah you can't do it the way you have it coded. The reason is like you found in your first comment, I do use the bootstrap-select with the changed.bs.select trigger to call a selectedValue and selectedItem, I don't see any other way for my plugin to get the value and fill in both my selectedValue and selectedItem.

So at the end of the day, I think you just use the wrong function because those are the one I use in my plugin. There's 2 ways of dealing with this:

By using the following View

<abp-select element.bind="mypicker" 
   selected-item.bind="selectedCondimentItem" 
   selected-value.bind="selectedCondimentValue"
   collection.bind="allCampingStuff">
</abp-select>

1- get the value from the picker element with e.target.value like this

  mypickerChanged() {
    this.mypicker.events.onChanged = (e) => console.log('onChange ' + e.target.value);
  }

2- or use the valueChanged or itemChanged like this

  @bindable selectedCondimentItem;
  @bindable selectedCondimentValue;

  selectedCondimentItemChanged(newValue) {
    console.log(newValue)
  }
  selectedCondimentValueChanged(newValue) {
    console.log(newValue)
  }

The 2- is the preferable since getting it does work better when you get an array of objects. Oh I saw that you had the value.bind in your sample, please don't use it as I just didn't connect that at all and probably won't return anything valid.

Finally, I don't think there's anything wrong with my plugin as long as you use the correct bindable. However I see that this information might be missing in the README, so let me update it.

If this fixes your problem then please close the issue, if not then let me know. Thanks

ghiscoding commented 6 years ago

Please read the new section in the README use it in the ViewModel, let me know if it's not clear enough, I can update the doc. Thanks

Mobe91 commented 6 years ago

Thank you for your detailed answer! Your 2nd approach is also the approach that I ended up using. I think your changes to the README are very clear. Maybe you could also write about how one would register such an observer for a field inside a complex object:

<abp-select element.bind="mypicker" 
   selected-item.bind="condimentSelection.selectedCondimentItem" 
   selected-value.bind="condimentSelection.selectedCondimentValue"
   collection.bind="allCampingStuff">
</abp-select>
@autoinject
export class MyController {
   bindingEngine: BindingEngine;
   allCampingStuff = [...]; // some collection
   condimentSelection = {
      selectedCondimentItem: null,
      selectedCondimentValue: null
   };
   selectedCondimentItemSubscription: Subscription;

   constructor(bindingEngine: BindingEngine) {
    this.bindingEngine = bindingEngine;
   }

   attached() {
    let observer = this.bindingEngine.expressionObserver(this.condimentSelection, 'selectedCondimentItem');
    this.selectedCondimentItemSubscription= observer.subscribe(newValue => this.selectedCondimentItemChanged(newValue));
   }

   detached() {
    this.businessesSubscription.dispose();
   }

   selectedCondimentItemChanged(newValue) {
     // handle value change
   }
}
ghiscoding commented 6 years ago

Added your sample in the readme, How to use it with a complex object?, thanks for that.

Closing since it's now resolved. If you like the plugin(s), you can also upvote :star: thanks!