dfilatov / vidom

Library to build UI based on virtual DOM
MIT License
415 stars 16 forks source link

select multiple confirmed not working #65

Closed ghost closed 8 years ago

ghost commented 8 years ago

Hi. I know I'm right in everything I write to you, but... Anyhow. Select multiple comfirmed not working.

 var node = createNode("select")
                    .attrs({ multiple: "multiple", value: ["foo", "bar"] })
                    .children([
                            createNode("option").attrs({ value : "foo" }).children("foo"),
                            createNode("option").attrs({ value : "bar"}).children("bar")
                        ])

     var hardHeaded = node.renderToDom();

If you now write come correct code, and try to get the results. It will be like this:

Your output: 'foo

Expected output: [ 'bar', 'foo' ]

voischev commented 8 years ago

Это стеб какой то? :)

ghost commented 8 years ago

Просто попробуйте. Это не работает. Там нет массивом.

ghost commented 8 years ago

@dfilatov Confirmed also that this is not working if you force it to be an property. So multiple select are not working either as a property or a attribute.

ghost commented 8 years ago

@dfilatov If you try to populate the value property on select multiple using groups, it will not work either.

ghost commented 8 years ago

@dfilatov Any progress on solving this issue?

dfilatov commented 8 years ago

Should work now. But I should warn you vidom supports only case to specify value in the select — specifying value as an attribute of "select" node. Other ways like "selectedIndex" or specifying of attrubute "selected" in option aren't supported and it's not going to be supported.

Regular select:

createNode("select")
    .attrs({ value : 5 })
    .children([
        createNode("optgroup").attrs({ label : 'group1' }).children([
            createNode("option").attrs({ value : 1 }).children('1'),
            createNode("option").attrs({ value : 2 }).children('2'), 
            createNode("option").attrs({ value : 3 }).children('3')
    ])
]);

Multiple select:

createNode("select")
    .attrs({ multiple : true, value : [2, 3] })
    .children([
        createNode("optgroup").attrs({ label : 'group1' }).children([
            createNode("option").attrs({ value : 1 }).children('1'),
            createNode("option").attrs({ value : 2 }).children('2'),
            createNode("option").attrs({ value : 3 }).children('3')
   ])
]);
ghost commented 8 years ago

@dfilatov That's ok, but this is still not working 100%. And if the basic are not working, you will get a lot of issue tickets. Even jQuery supports the basics.

See my first post in this thread. That is the basics, and not working!!

dfilatov commented 8 years ago

What exactly is not working? Could you provide not working example? Your code in the top message is working.

ghost commented 8 years ago

My code in top returning "foo" if I test it with your code. It should return an array with "bar" and "foo". Because both "bar" and "foo" are selected.

In your code, you are patching from select to select multiple with numbers. That will work.

Add to the record. I'm using my own function to grab the result of the node to get this output. And my function is equal to jQuery, Angular, REACT, etc. So nothing wrong with that one.

dfilatov commented 8 years ago

I don't understand you. Do you want that native DOM element returns array as its value???

ghost commented 8 years ago

@dfilatov Here is my private code I use for checking this.

Try this code and let me know the result you get IF you do this:

getSelectValue(hardHeaded).sort()

What result do you get? You see that in the top code "foo" and "bar" are set as an array. Meaning they both should be marked as seleced on the two child nodes.

Meaning, it should return an array containing both "bar" and "foo", but with your code you are ONLY selecting the "foo".

Here is my code


function getSelectValue(node) {

        var type = node.getAttribute("type") == null ? node.nodeName.toLowerCase() : node.getAttribute("type");

        if (arguments.length === 1) {

             if (type === "select") {

                if (node.multiple) {

                    var result = [];
// USE ARRAY.FOREACH HERE!!
                    each(node.options, function(option) {

                        if (option.selected &&
                            // Don't return options that are disabled or in a disabled optgroup
                            option.getAttribute("disabled") === null &&
                            (!option.parentNode.disabled || option.parentNode.nodeName !== "OPTGROUP")) {
                            result.push(option.value || option.text);
                        }
                    });

                    return result;
                }
                return ~node.selectedIndex ? node.options[node.selectedIndex].value : "";
            }
        }

        var ret = node.value;
        return typeof ret === "string" ?
            // Handle most common string cases
            ret.replace(rreturn, "") :
            // Handle cases where value is null/undef or number
            ret == null ? "" : ret;
    };
dfilatov commented 8 years ago

Here is screenshot of your example: screen shot 2015-08-03 at 19 26 31 Both options are selected.

ghost commented 8 years ago

If you are running the top code with my code, and you get that result. Then something is wrong. And I need to figure out if there is a browser issuse, or OS issue.

dfilatov commented 8 years ago

I tried to run your code and got: Array [ "foo", "bar" ], so you're doing something wrong. Maybe you've forgotten to update git repo, haven't you?

ghost commented 8 years ago

No. There are something very strange here. With the code on top and my solution code, it's impossible you can get it to work. It will never return an array!!

option.selected are always false because it's not set. And then it will not return an array, only single value.

If I try this: option.getAttribute("select") I get null Because it's not an attribute either.

But hold on. It's easy to get it working, but require that you fix your code. Because your setSelectValue function are wrong if you want this to work.

A working code would be similar to this

    options = elem.options,
                    values = makeArray( value ),
                    i = options.length;

                while ( i-- ) {
                    option = options[ i ];
                    if ( (option.selected = isInArray( option.value, values ) >= 0) ) {
                        optionSet = true;
                    }
                }

                // Force browsers to behave consistently when non-matching value is set
                if ( !optionSet ) {
                    elem.selectedIndex = -1;
                }
                return values;
dfilatov commented 8 years ago

I run your top code with getSelectValue in firefox, chrome and safari and everywhere got the expected result. Which browser do you test in?

ghost commented 8 years ago

Finaly!! Got it working. Issue was you wrapped a condition inside a parenthese where the browser got confused. I tried various browsers. Latest was older Android and iOS

ghost commented 8 years ago

This line: optionNode.selected = value != null && (isMultiple? isInArray(value, optionNode.value) : optionNode.value == value); changed to

optionNode.selected = value != null && isMultiple ? isInArray(value, optionNode.value) : optionNode.value == value;

Then it works. As you see isMultiple belongs to && outside the parenthese

dfilatov commented 8 years ago

no, your variant is wrong. there's no need to check the second part if value is equal to null.