Remove id dependency #1360

pfiller · 2013-07-16T22:00:39Z

Some time ago, we made a change in Chosen that made the plugin less dependent on HTML IDs. This had two major benefits: 1) the code became less brittle and 2) it became possible to initialize Chosen outside of a DOM context. Chosen still has some remnants of this ID requirement and I've finally gotten around to cleaning them up in this PR.

Notes about this PR

IDs are no longer required on any element within Chosen.
The function that generates random IDs for jQuery selects has been ripped out.
If the select Chosen is being applied to has an ID already, Chosen still adds an ID to its container as I have found this functionality useful.
There is a lot of red in this PR 💥

there's no need to manually update the visibility of an option.

Removing ID dependencies will allow us to remove some ugly code down the road. I like that.

a requirment for Chosen to function. If a select has an ID, give Chosen an appropriately named ID. If it doesn't have an ID, keep calm and carry on.

kenearley · 2013-07-16T22:18:47Z

coffee/chosen.jquery.coffee

@@ -152,7 +150,7 @@ class Chosen extends AbstractChosen


  test_active_click: (evt) ->
-    if $(evt.target).parents('#' + @container_id).length
+    if @container[0] is $(evt.target).parents('.chzn-container')[0]


Probably not a big deal, but you can also access the element with @container.get(0)

instead of getting all parents matching .chzn-container and getting the first one in it, it would be more efficient to use .closest('.chzn-container') so that it stops traversing the DOM once it finds the first one

I actually prefer the array syntax to get(0).

c335d5f switches to closest

it finds a result.

stof · 2013-07-16T22:38:20Z

coffee/chosen.jquery.coffee

@@ -152,7 +150,7 @@ class Chosen extends AbstractChosen


  test_active_click: (evt) ->
-    if $(evt.target).parents('#' + @container_id).length
+    if @container[0] is $(evt.target).closest('.chzn-container')[0]


you don't need the [0]. You can pass the jquery collection to .is(). The collection return by .closest has at most as many elements as the original collection anyway (as it can find 0 or 1 closest matching parent for each element in the collection) so it will only contain 0 or 1 element here.

That looks much nicer.

if @container.is($(evt.target).closest('.chzn-container'))

ad2de99

kenearley · 2013-07-17T15:19:12Z

coffee/lib/abstract-chosen.coffee

@@ -87,11 +85,10 @@ class AbstractChosen

    style = if option.style.cssText != "" then " style=\"#{option.style}\"" else ""


This line is adding the style attribute with no value when there is no cssText. Could we do this?

style = if option.style.cssText? "" then " style=\"#{option.style}\"" else ""

Also, I'd love to get rid of the double quote escaping and just use single quote.

I don't really like changing unrelated lines. We can open a separate PR

kenearley · 2013-07-17T15:27:36Z

This looks good to me. Just one small comment above.

Remove Chosen's ID dependency

- Search optgroups #1343 - Change search match style #1344 - Remove memoization of search field width. #1359 - Remove Chosen's ID dependency #1360 - Rebuild Search Results After liszt:updated #1361 - Clear highlight when no results are found #1364

pfiller added 8 commits July 16, 2013 14:52

Group ID's are no longer used. Stop adding them to the HTML

251310e

results_option_build is calle after a result is deselected so

23cc35c

there's no need to manually update the visibility of an option.

Remove id dependency from test_active_click

baba462

Use a data- attribute to link an option with its chosen HTML element.

0d61a5a

Removing ID dependencies will allow us to remove some ugly code down the road. I like that.

No longer add an ID for each option.

5eb560c

Stop generating random IDs for ID-less selects as ID is no longer

3bae381

a requirment for Chosen to function. If a select has an ID, give Chosen an appropriately named ID. If it doesn't have an ID, keep calm and carry on.

Remove the (now unused) ID generation functionality.

64ae331

Merge branch 'master' into remove-id-dependency

f510802

kenearley reviewed Jul 16, 2013
View reviewed changes

pfiller added 2 commits July 16, 2013 18:26

Use closest instead of parents because closest short circuits once

c335d5f

it finds a result.

Rename array_index data attribute for clairty and conformity.

3721468

stof reviewed Jul 16, 2013
View reviewed changes

.closest finds 0 or 1 closest matching parent - update syntax

ad2de99

kenearley reviewed Jul 17, 2013
View reviewed changes

pfiller added a commit that referenced this pull request Jul 17, 2013

Merge pull request #1360 from harvesthq/remove-id-dependency

7e59f83

Remove Chosen's ID dependency

pfiller merged commit 7e59f83 into master Jul 17, 2013

pfiller deleted the remove-id-dependency branch July 17, 2013 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove id dependency #1360

Remove id dependency #1360

pfiller commented Jul 16, 2013

kenearley Jul 16, 2013

stof Jul 16, 2013

pfiller Jul 16, 2013

stof Jul 16, 2013

pfiller Jul 17, 2013

kenearley Jul 17, 2013

kenearley Jul 17, 2013

pfiller Jul 17, 2013

kenearley commented Jul 17, 2013

		@@ -87,11 +85,10 @@ class AbstractChosen

		style = if option.style.cssText != "" then " style=\"#{option.style}\"" else ""

Remove id dependency #1360

Remove id dependency #1360

Conversation

pfiller commented Jul 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenearley commented Jul 17, 2013