paulstraw / FancySelect

A better select for discerning web developers everywhere.
http://code.octopuscreative.com/fancyselect/
MIT License
754 stars 165 forks source link

Question regarding click event & chaining. #70

Closed alycda closed 6 years ago

alycda commented 9 years ago

Hi,

I see that in commit 07cc726 the native trigger('change') event was removed, but not re-added when you finalized namespacing. I've added it back locally, but I have questions about the function's need/ability for chaining.

options.on('click.fs', 'li', function (e) {
    var clicked;
    clicked = $(this);
    if (sel.val() !== clicked.data('raw-value')) {
        sel.val(clicked.data('raw-value')).trigger('change');
    }
    if (!isiOS) {
        sel.trigger('blur.fs').trigger('focus.fs');
    }
    return sel.val(clicked.data('raw-value')).trigger('change.fs').trigger('blur.fs').trigger('focus.fs');
}

I added a conditional to determine whether the old and new values were the same, so as not to trigger a change event when the value hasn't actually changed. Looking further into the code though, I was wondering why you have a conditional for iOS, but then trigger the blur() & focus() events anyway in the return statement.

if (!isiOS) {
    sel.trigger('blur.fs').trigger('focus.fs');
}
return sel...trigger('blur.fs').trigger('focus.fs');

I'm wondering if your code wouldn't benefit from the following refactor:

options.on('click.fs', 'li', function (e) {
    var clicked;
    clicked = $(this);

    if (sel.val() === clicked.data('raw-value')) {
        sel.trigger('close:fs');
    } else {
        sel.val(clicked.data('raw-value'));
        sel.trigger('change'); // this could be moved to the el.on('change.fs') listener

        if (!isTouch) {
            sel.trigger('blur.fs').trigger('focus.fs');
        }

        options.find('.selected').removeClass('selected');
        clicked.addClass('selected');
        trigger.addClass('selected');
    }
}

I guess there may be an issue with chaining here, what are your thoughts?