ibm-js / deliteful

Multichannel (desktop/mobile) UI Custom Elements Library
http://ibm-js.github.io/deliteful
Other
70 stars 36 forks source link

undefined _storeAdapter after "Various fixes to Combobox refactor" #719

Closed lindgr3n closed 4 years ago

lindgr3n commented 4 years ago

Hi! Im using master (6cc397814f720d396d9242d6276885015f270dfb) (Also latest checkin of delite)

Im creating a new Combobox like this

var list = new List({ source: data }); // data is an array with objects [{}]
var cmb = new Combobox({
          searchPlaceHolder: '',
          list: list,
          value: data[0].label,
          selectionMode: 'single',
          class: ''
 });
element.appendChild(cmb);

Im getting a error in getIdentity were _storeAdapter is undefined.

getIdentity: function (item) {
    return this._storeAdapter.getIdentity(item);
}

If I revert the change in ComboboxImplementation.js made in "Various fixes to Combobox refactor." (6186284ed2c62f58fecf9cf57c3cbbec9c8b21c0) and move the connectedCallback call back to the top of the if block it works again.

_initList: function () {
    if (this.list) {
        // TODO
        // This is a workaround waiting for a proper mechanism (at the level
        // of delite/Store - delite/StoreMap) to allow a store-based widget
        // to delegate the store-related functions to a parent widget (delite#323).
        if (!this.list.attached) {
            this.list.connectedCallback();
        }

        // Class added on the list such that Combobox' theme can have a specific
        // CSS selector for elements inside the List when used as dropdown in
        // the combo.
        this.list.classList.add("d-combobox-list");

                // The drop-down is hidden initially
        this.list.classList.add("d-hidden");

                // The role=listbox is required for the list part of a combobox by the
        // aria spec of role=combobox
        this.list.type = "listbox";

                this.list.selectionMode = this.selectionMode === "single" ?
            "radio" : "multiple";

        this._initHandlers();
    }
},
wkeese commented 4 years ago

Hmm, I don't remember exactly why I switched the order of things in _initList(), but I'm pretty sure it was to fix some failure, the kind of problem you are ironically getting now.

I tried the test case you pasted above but it's working for me, or at least, no exception. I see that storeAdapter is initialized in Store#queryStoreAndInitItem() which I would think would be called before getIdentity().

What's the full stack of your failure? And/or, can you give a full test case?

wkeese commented 4 years ago

PS: BTW, if that's really how your code looks, then why not just pass source directly to Combobox, rather than manually creating a List?

lindgr3n commented 4 years ago

Thanks for the quick response! Its kinda how it looks. Hard to make a minimal example right now.

Good question. Don't think i know what the difference would be :) Just following how it have been done in other places in the code. And the examples in deliteful/docs/Combobox.md all uses new List.

Tried your suggestion and skipped the list

 var cmb = new Combobox({
          searchPlaceHolder: '',
          source: data,
          value: data[0].label,
          selectionMode: 'single',
          class: ''
        });

Got problem then with cmb.list.deliver() (Missed that part in the initial example) Removed it and looks like it works!

Tried some more and it looks like the problem was using cmb.list.deliver(); after creating the instance.

var cmb = new Combobox(...)
cmb.list.deliver();
element.appendChild(cmb)

Removed the deliver and my intial example with new List works also. Any documentation on when one need to use deliver()?

Think the use of deliver on list was to initialize the list to be able to set a default value cmb.list.selectedItem = data[0];

wkeese commented 4 years ago

I'm confused because your initial example didn't call List#deliver(). Are you saying that Combobox doesn't work unless you call List#deliver()?

Think the use of deliver on list was to initialize the list to be able to set a default value cmb.list.selectedItem = data[0];

Yes, not "default" value exactly, but selected value. Ideally, you should never need to call deliver(), but IIRC Combobox is naughty and mucks directly with the List items' internal nodes, specifically setting selected/unselected status. deliver() makes sure those internal nodes are rendered.

And the examples in deliteful/docs/Combobox.md all uses new List.

Yes... that was the original design of Combobox but it made no-sense performance-wise because all the data had to be queried and rendered at page load. The data shouldn't be queried or rendered until the user manually opens the Combobox dropdown, or starts typing a search string that causes the dropdown to open automatically.

lindgr3n commented 4 years ago

Are you saying that Combobox doesn't work unless you call List#deliver()?

It don't work when i use List#deliver(). If i remove List#deliver() it works :) Forgot to add that row in my initial example that I used it.

This works

var list = new List({ source: data });
var cmb = new Combobox({
    searchPlaceHolder: '',
    // source: data, // Also works insted of using list
    list: list,
    value: data[0].label,
    selectionMode: 'single',
    class: ''
});
// cmb.list.deliver();  // This don't work
element.appendChild(cmb);

Thanks for the help!

Feels a bit strange using value: data[0].label, Is that the correct way of setting selected (default) value?

wkeese commented 4 years ago

Oh OK, glad it's working.

About data[0].label, the way you have it is correct. The reason it seems weird is that (as I was saying above) the new Combobox API is optimized to defer querying the store (aka source) until necessary. So in general, you won't have a data array during Combobox creation, and it will look more like this:

var cmb = new Combobox({
    source: asyncSource
    value: 12345,
    displayedValue: "France"
});

In other words, the server will send down the initial value/displayedValue, but asyncSource won't be queried until the user interacts with the Combobox.