mysociety / jquery-multi-select

Converts <select multiple> elements into dropdown menus with checkboxes
https://www.mysociety.org/
Other
24 stars 18 forks source link

Clicking the item's label in one multiselect is firing the change event on a different multiselect control #33

Closed norrelr closed 2 years ago

norrelr commented 2 years ago

I have about a dozen multiselect dropdowns on a form and when I click on the label portion of an item, not the check box itself, it causes the change event on a different multiselect to change. Actually all of the multiselects cause the change event of the first multiselect on the page to update. I thought it was the additional change events I was adding but when I took those off it was still happening so it appears to be something in the change event of the control.

zarino commented 2 years ago

@norrelr That’s odd! The demo features multiple selects on a single page, and the labels work fine.

Is it possible that your <select> elements are all sharing the same id attribute?

If not, perhaps you could provide a link to your code, or a minimal version that exhibits the bug, so we could reproduce it?

norrelr commented 2 years ago

Ok, right, the selects all have a different id. I will review the demo again and then if I still have the issue I will strip the page down to just these controls so that I can share it with you. I am new to github, we use azure devops. what is the best way for me to share the project with you? should I create a repo in github and put it there? just now sure what the best way to do that is. I will probably find the issue while creating this stripped down version. Thanks for your prompt reply!

norrelr commented 2 years ago

So, I was able to find the issue with the conflict between my code and the multi-select.js. Problem was that I did not have a name attribute on the multi select controls so the unique_id for the options were the same between the controls. Then when the change event was firing it would reference the first one with that name. Added the name attribute to them and that fixed the issue. Here is the piece of code in the js.

constructMenuItem: function ($option, option_index) {
    var unique_id = this.$element.attr('name') + '_' + option_index;

I am not really deep on html but I would my thought is that I know the ID attribute is supposed to be unique so maybe this code should use that value instead of the name. I have grown accustomed to referencing all my controls by ID so I often don't give them names. Looks like that might be a bad practice on my part.

Thanks

zarino commented 2 years ago

Ah, that’s useful to know, thanks for investigating @norrelr!

It looks like we use this.$element.attr('name') in a few places. Does make me wonder whether we should be using this.$element.attr('id') instead… 🤔

norrelr commented 2 years ago

Thanks. I think it is common fairly practice to have a name attribute but I would think having an id would be required but neither is. Maybe throw an error is you don’t find the name attribute.

I am fairly new to finding and using these sorts of components and am really liking the ability to do this. Thanks for making it available, real time savor for me. Just added the noneText and allText presets yesterday, fantastic!

Richard Norrell ECM Team Supervisor Solutions Development & Modernization Team @. @*.**@*.***> | (360) 705-7632

From: Zarino Zappia @.> Sent: Wednesday, March 30, 2022 12:56 AM To: mysociety/jquery-multi-select @.> Cc: Norrell, Richard @.>; Mention @.> Subject: [EXTERNAL] Re: [mysociety/jquery-multi-select] Clicking the item's label in one multiselect is firing the change event on a different multiselect control (Issue #33)

WARNING: This email originated from outside of WSDOT. Please use caution with links and attachments.

Ah, that’s useful to know, thanks for investigating @norrelrhttps://github.com/norrelr!

It looks like we use this.$element.attr('name') in a few places. Does make me wonder whether we should be using this.$element.attr('id') instead… 🤔

— Reply to this email directly, view it on GitHubhttps://github.com/mysociety/jquery-multi-select/issues/33#issuecomment-1082749456, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AYORDEK6LH5VING3NX7SP5DVCQCKBANCNFSM5R46SLLQ. You are receiving this because you were mentioned.Message ID: @.**@.>>