Skip to content
New issue

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

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

Already on GitHub? Sign in 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants