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 #6193, change event should not be triggered if each value still matches an option #6194

Merged
merged 3 commits into from
Aug 29, 2017

Conversation

jkzing
Copy link
Member

@jkzing jkzing commented Jul 23, 2017

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
#6193


It seems we still need the value matching check, only detect options change is not enough.

@jkzing jkzing force-pushed the fix-6193 branch 3 times, most recently from e90d6d1 to d9b2f41 Compare July 23, 2017 14:38
change event should not be triggered if still can found
matching option for each value
@@ -101,6 +108,15 @@ function setSelected (el, binding, vm) {
}
}

function hasNoMatchingOption (value, options) {
Copy link
Member

@Kingwl Kingwl Jul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about Array.prototype.some or every😅

Copy link
Member Author

@jkzing jkzing Jul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would be better to use [].some.
Actually I just did revert changes made in c90b140, and didn't think about that. 😅
Will do it later.

@cgarnier
Copy link

Is it released ?

@LinusBorg
Copy link
Member

It hasn't been merged yet, so: no.

@yyx990803 yyx990803 merged this pull request into vuejs:dev Aug 29, 2017
@jkzing jkzing deleted the fix-6193 branch August 30, 2017 02:57
FranklinTesla pushed a commit to FranklinTesla/vue that referenced this pull request Sep 5, 2017
ztlevi pushed a commit to ztlevi/vue that referenced this pull request Feb 14, 2018
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants