vrimar / construct-ui

A Mithril.js UI library
https://vrimar.github.io/construct-ui
MIT License
287 stars 23 forks source link

Select with onchange handler doesn't repaint the select #5

Closed angrytongan closed 4 years ago

angrytongan commented 4 years ago

The construct-ui Select element with an onchange handler fails to repaint the element correctly after a new selection has occurred.

The code below creates two construct-ui Select elements with one mithril select element. All three select elements reference the same scoped variable for the selected value.

Expected behaviour: changing one select element should change all select elements.

Actual behaviour: changing the mithril select element changes all select elements which is correct. Changing one of the construct-ui Select elements changes the mithril select element and the other construct-ui Select element, but sets value of the chosen construct-ui Select element to the first element in the list.

Removing the onchange handler from one of the construct-ui Select elements is fine; selected element is repainted correctly.

Running a fresh build from the 1.0 branch, with mithril.js 2.0.4. Tested with latest Chrome and Safari on Mac OS X 10.15.1.

I am new to mithril.js and construct-ui so may be doing something basic incorrectly. Any pointers or advice would be very welcome.

'use strict';

const {
    FocusManager,
    Select,
} = CUI;
FocusManager.showFocusOnlyOnTab();

const BrokenSelect = () => {
    let selected = 0;

    return {
        view: () => {
            return m('', [
                m(Select, {
                    options: [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ],
                    onchange: (e) => { selected = e.currentTarget.value; },
                    value: selected,
                }),

                m(Select, {
                    options: [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ],
                    onchange: (e) => { selected = e.currentTarget.value; },
                    value: selected,
                }),

                m('select', {
                    value: selected,
                    onchange: (e) => { selected = e.currentTarget.value; },
                }, [
                    m('option', { value: 0 }, '0'),
                    m('option', { value: 1 }, '1'),
                    m('option', { value: 2 }, '2'),
                    m('option', { value: 3 }, '3'),
                    m('option', { value: 4 }, '4'),
                    m('option', { value: 5 }, '5'),
                    m('option', { value: 6 }, '6'),
                    m('option', { value: 7 }, '7'),
                    m('option', { value: 8 }, '8'),
                    m('option', { value: 9 }, '9'),
                ]),
            ]);
        }
    };
};

document.addEventListener('DOMContentLoaded', () => {
    m.route(document.body, '/', {
        '/': BrokenSelect,
    });
});
angrytongan commented 4 years ago

Also: having just a single construct-ui Select element also demonstrates this behaviour.

'use strict';

const {
    FocusManager,
    Select,
} = CUI;
FocusManager.showFocusOnlyOnTab();

const BrokenSelect = () => {
    let selected = 0;

    return {
        view: () => {
            return m('', [
                m(Select, {
                    options: [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ],
                    onchange: (e) => { selected = e.currentTarget.value; },
                    value: selected,
                }),

            ]);
        }
    };
};

document.addEventListener('DOMContentLoaded', () => {
    m.route(document.body, '/', {
        '/': BrokenSelect,
    });
});
vrimar commented 4 years ago

Should be fixed in latest commit.

vrimar commented 4 years ago

I need to add onchange tests for the select component as well, thanks for the report!

angrytongan commented 4 years ago

Thanks for the fast fix - all good. Awesome library, thanks so much for all your work!