Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix issue #4275 #4366

Closed
wants to merge 2 commits into from
Closed

Fix issue #4275 #4366

wants to merge 2 commits into from

Conversation

vinh123456789
Copy link

Fix issue #4275, when using useLabels: false, dropdown("clear") won't
remove class "active" from "item"

Fix issue #4275, when using useLabels: false, dropdown("clear") won't
remove class "active" from "item"
@davidkuhta
Copy link

If we're calling module.remove.activeItem(); inside both the if and the else, shouldn't it be brought outside?
Something like:

clear: function() {
  if(module.is.multiple()) {
    module.remove.labels();
  }
  else {
    module.remove.selectedItem();
  }
  module.remove.activeItem();
  module.set.placeholderText();
  module.clearValue();
},

@vinh123456789
Copy link
Author

vinh123456789 commented Jul 31, 2016

You are right...
This is my first time contributions to a github project, I was so nervous that I would make a mistake, and here is it...

@davidkuhta
Copy link

Well technically it wasn't a mistake because your code worked, we just refactored. 😄
And whatever way @jlukic goes with this PR, thanks for helping to make Semantic UI better.

@jlukic
Copy link
Member

jlukic commented Jul 31, 2016

remove labels calls remove values so this would cause values to remove twice when labels are present.

Probably need

clear: function() {
  if(module.is.multiple() && settings.useLabels) {
    module.remove.labels();
  }
  else {
    module.remove.selectedItem();
  }
  module.remove.activeItem();
  module.set.placeholderText();
  module.clearValue();
}

@jlukic jlukic added this to the 2.2.3 milestone Jul 31, 2016
jlukic added a commit that referenced this pull request Jul 31, 2016
@jlukic jlukic closed this Jul 31, 2016
@jlukic
Copy link
Member

jlukic commented Jul 31, 2016

Confirmed fix

jlukic added a commit that referenced this pull request Jul 31, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants