harvesthq / chosen

Deprecated - Chosen is a library for making long, unwieldy select boxes more friendly.
http://harvesthq.github.io/chosen/
Other
21.85k stars 4.1k forks source link

chosen: updated does not pick up css-class. #2232

Open qwelyt opened 9 years ago

qwelyt commented 9 years ago

TL;DR: As can be seen in this fiddle: http://jsfiddle.net/73qm0whz/

Issue: chosen: update does not pick up on changed css-classes. If a class has been added dynamically via JS and you after that do the handy $("#idtag").trigger("chosen: updated"); command, Chosen will not pick up the new class. It will pick up on changed options, placeholders and whatnots, but the css-class is never read.

ComeBurguburu commented 9 years ago

The function results_update_field should called this.set_up_html instead of this.results_build. Could you try this fix and make a pull request ?

qwelyt commented 9 years ago

@ComeBurguburu That fix does not work. By using set_up_html just like that, you get the very odd behaviour of getting new select-boxes whenever you tell chosen to update. But it still seems that it does not pick up the changed class.

ComeBurguburu commented 9 years ago

That weird because set_up_html is the method who pickup the css-classes. If you change css-classes you need to rebuild your select not update it:

$(my_selector).chosen("destroy").chosen(options)

qwelyt commented 9 years ago

Doing that roundabout way seems to work. But it's a bit flawed in practice. If I have a bunch of selects, lets say 10, on a page and want to mass-update the css-class when the user does something, I don't want to have too keep track of what different options each single select has been initiated with. I would want to be able to tell chosen "update this select with all the options you have already been given". This would be possible if chosen provided some "getActivatedOptions" function that returned what options were used in the select. That way you could store those options before you destroyed the elements, and the pass the variable with chosen when you re-create it. But it would sitll be a round-about way of doing it. Why not just update the "updated"-option to do this right of the bat?

ComeBurguburu commented 9 years ago

Yeah I agree with you the better way should be to correct the updated. try to add those lines before calling set_up_html in "chosen:updated" case : this.container.remove(); this.form_field_jq.removeData('chosen');

or you can use a wrapper around your select and change the css if you only need this class to indicate that the selected value is wrong

can you share your updated chosen version in a jsfiddle ?

qwelyt commented 9 years ago

Right. I've come up with a butched hack of the results_build function.

    var addedClasses;
    Chosen.prototype.results_build = function() {
      this.parsing = true;
      this.selected_option_count = null;
      this.results_data = SelectParser.select_to_array(this.form_field);
      if (this.inherit_select_classes && this.form_field.className) {
          addedClasses = this.form_field.className;
          this.container.addClass(addedClasses);
      } else {
          this.container.removeClass(addedClasses);
      }
      if (this.is_multiple) {
        this.search_choices.find("li.search-choice").remove();
      } else if (!this.is_multiple) {
        this.single_set_selected_text();
        if (this.disable_search || this.form_field.options.length <= this.disable_search_threshold) {
          this.search_field[0].readOnly = true;
          this.container.addClass("chosen-container-single-nosearch");
        } else {
          this.search_field[0].readOnly = false;
          this.container.removeClass("chosen-container-single-nosearch");
        }
      }
      this.update_results_content(this.results_option_build({
        first: true
      }));
      this.search_field_disabled();
      this.show_search_field_default();
      this.search_field_scale();
      return this.parsing = false;
    };

So it takes the same check as the set_up_html does with inherit_select_classes and check if the select has any class on it. If it does, it adds that class to a variabel ( addedClasses ). The addedClasses variable is then added to the container. The reason for the variable? Because what if the class get's removed from the container? So if we have added a class to the container that has then been removed, we know what to remove from the container.

This works, but it's an ugly hack. And I don't know coffeescript, so there will be no PR for this. At least not from me.

ComeBurguburu commented 9 years ago

maybe we can just move the firsts lines who deals with style from set_up_html to results_build but we still have a problem with this line we can't move

      this.container = $("<div />", container_props);
isochronous commented 9 years ago

My first thought when looking at this is that the logic for copying css classes (and other attributes like title) from the select to the container should be abstracted out into a separate method. That should allow the original functionality to continue working as well as enabling other events to trigger the same behavior without all of the extra logic included in results_build or set_up_html.

I'm running into a similar problem but with the title attribute rather than the class attribute (#2311), so I may take a crack at this a little later today. I just wish I better understood their naming conventions - I'm not really sure when to use "result" and "results" as the method name prefixes.

isochronous commented 9 years ago

Okay, I've checked in a modification that I feel is relatively elegant: isochronous/chosen@7425638b41f4b8158dd9757d7a45369ffc3f9079. For some reason the actual compiled files don't seem to get checked in, but it's a simple matter of cloning the repo, navigating into the directory via command line, and running npm install && gem install bundler && bundle install. I had to run npm install -g grunt-cli as well, but then you just run grunt build and you'll get the pure JS version of both jQuery and Prototype versions. I've got to run right now but I'll check back in tomorrow and answer any questions.

Note that I also updated the jasmine tests pretty extensively, but I'm unfamiliar with prototype, and the last couple of prototype tests are throwing exceptions and I'm not sure why. The exact same tests work fine in the jQuery version though, so I'm not super concerned about it, but I would appreciate some help figuring out what the problem is before I submit a pull request to harvestHQ.

isochronous commented 9 years ago

Okay, I got all of the unit tests to pass for both jQuery and prototype versions and have submitted a pull request. In the meantime I've created a gist with fixed versions for both jQuery and prototype. https://gist.github.com/isochronous/541fa49aa34f7f5b20a3

isochronous commented 9 years ago

I have no idea what's going on with this repo, I've had the pull request in for three weeks now and not even a comment by the owners.

tjschuck commented 9 years ago

@isochronous All of the maintainers of Chosen fill that role voluntarily and as time allows. None of the maintainers is employed to work on open-source software full time. We all have other responsibilities (and lives!).

That said, the source of Chosen is completely open, as is your patch. Anyone that chooses to change Chosen's source with your modifications is fully free to do so at their own discretion. When a maintainer is able and willing to fully review your pull request, they will do so, but there's no guarantee — implicit or otherwise — that we will ever do that. Such is open-source. Please have some empathy and realize that there are humans with needs, wants, and dreams on both ends of this interaction.

You have to trust me that we all truly do appreciate your contribution, without even having necessarily looked at it. I realize that you took time away from your other responsibilities and life to work on the patch, and I'm grateful. Chosen is made better because of contributors like you.

Your pull request has not been closed; it has not been dismissed out of hand. It has merely not yet been fully reviewed for inclusion.

isochronous commented 9 years ago

I'm aware of that - I'm sorry if my comment came off as a complaint, but it wasn't intended as such. I was honestly just trying to get some kind of response back, as I was watching two issues and a pull request, and it seemed like no one was paying attention. I just wanted to make sure I wasn't yelling into the void - I'd have been just as happy with a "we're all very busy right now but will look at your pull request when we have some time to put towards chosen." So, basically, thanks for letting me/us know that there's someone paying attention :)

On Fri, May 22, 2015 at 10:43 AM T.J. Schuck notifications@github.com wrote:

@isochronous https://github.com/isochronous All of the maintainers of Chosen fill that role voluntarily and as time allows. None of the maintainers is employed to work on open-source software full time. We all have other responsibilities (and lives!).

That said, the source of Chosen is completely open, as is your patch. Anyone that chooses to change Chosen's source with your modifications is fully free to do so at their own discretion. When a maintainer is able and willing to fully review your pull request, they will do so, but there's no guarantee — implicit or otherwise — that we will ever do that. Such is open-source. Please have some empathy and realize that there are humans with needs, wants, and dreams on both ends of this interaction.

You have to trust me that we all truly do appreciate your contribution, without even having necessarily looked at it. I realize that you took time away from your other responsibilities and life to work on the patch, and I'm grateful. Chosen is made better because of contributors like you.

Your pull request has not been closed; it has not been dismissed out of hand. It has merely not yet been fully reviewed for inclusion.

— Reply to this email directly or view it on GitHub https://github.com/harvesthq/chosen/issues/2232#issuecomment-104677810.

tjschuck commented 9 years ago

@isochronous Thanks for understanding :heart: :heart: :heart_eyes_cat: :heart:

isochronous commented 9 years ago

And then I found this: https://github.com/harvesthq/chosen/pull/166

And my hopes of ever seeing this pull request merged vanished like smoke in the wind.