fix(select): when using trackBy, trigger ng-change only when tracked property is different #11156

chenlijun99 · 2018-03-10T00:23:14Z

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

When using ng-model-options="{ 'trackBy': '$value.someProperty' }" on md-select, ng-change is triggered when the selected option's object is not equal to the one of the model (the comparison is done with angular.equals which compares recursively all the properties of the object).
Issue Number: #11108

What is the new behavior?

ng-change is triggered when the tracked property of the model is different from the selected option.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Splaktar

Thank you for sending this PR!

I just had the one minor typo comment. Manual testing looks good.

One other thing, can you please update the commit to more closely follow the commit guidelines especially with the Closes line being on the last line? Thank you.

src/components/select/select.js

chenlijun99 · 2018-03-13T14:45:09Z

@Splaktar Do you have any clue on why is the travis ci build failing?
I've only fixed the typo and changed the commit message.

It seems to be failing on other pull requests as well.

Splaktar · 2018-03-13T19:01:58Z

TravisCI had a significant outage last night around that time. I've restarted the tests.

Splaktar · 2018-03-13T19:03:55Z

I need to improve the commit message guidelines, sorry for the confusion. I'll try to add some examples to them.

I believe that the footer needs to use the format Closes #11108 and not Closes issue #11108. This is how the feature of GitHub works.

chenlijun99 · 2018-03-13T19:06:41Z

I've changed the commit message.

But the travis ci build seems to be still failing.

Splaktar · 2018-03-13T19:06:48Z

It also looks like there is an issue with the AngularJS snapshot builds. I'll check with the team.

Splaktar

LGTM

Splaktar · 2018-03-13T19:10:03Z

It's possible that their snapshot somehow got corrupted by the TravisCI outage. I'll let you know when I find out more.

chenlijun99 · 2018-03-14T09:08:17Z

Which would be the best way to integrate this fix into my project before the next version of angular material is released?

Splaktar · 2018-03-14T23:07:12Z

Once we get it merged, there will be a build with a hash on it that you will be able to install directly via NPM.

src/components/select/demoTrackBy/index.html

gkalpak · 2018-03-15T12:32:41Z

src/components/select/select.js

+            // if they have the same length, then they are different
+            // if an item in the newVal array can't be found in the prevVal
+            return newVal.every(function(newValItem) {
+              return !!prevVal.find(function(prevValItem) {


Not sure what the supported browsers are, but make sure they all implement .find() (IE does not for example).

Gosh! I've always thought find was supported since IE 9...

src/components/select/select.js

Concerns have been addressed.

Splaktar · 2019-05-18T18:46:13Z

It looks as though the internal issues have been resolved and this may be able to get into 1.1.19.

jelbourn

LGTM

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Mar 10, 2018

chenlijun99 force-pushed the fix-issue-11108 branch from 29ba62b to 239ee8e Compare March 10, 2018 10:18

Splaktar self-requested a review March 13, 2018 07:46

Splaktar self-assigned this Mar 13, 2018

Splaktar reviewed Mar 13, 2018

View reviewed changes

src/components/select/select.js Outdated Show resolved Hide resolved

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs type: bug P4: minor Minor issues. May not be fixed without community contributions. labels Mar 13, 2018

Splaktar added this to the 1.1.9 milestone Mar 13, 2018

chenlijun99 force-pushed the fix-issue-11108 branch from 239ee8e to a85b500 Compare March 13, 2018 13:52

chenlijun99 closed this Mar 13, 2018

chenlijun99 reopened this Mar 13, 2018

chenlijun99 force-pushed the fix-issue-11108 branch from a85b500 to 74413ee Compare March 13, 2018 19:05

Splaktar removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Mar 13, 2018

Splaktar approved these changes Mar 13, 2018

View reviewed changes

Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Mar 13, 2018

Splaktar modified the milestones: 1.1.9, 1.1.8 Mar 13, 2018

chenlijun99 force-pushed the fix-issue-11108 branch from 74413ee to 41a67b2 Compare March 14, 2018 07:53

chenlijun99 force-pushed the fix-issue-11108 branch from 41a67b2 to 7a7bc3d Compare March 14, 2018 14:50

gkalpak reviewed Mar 15, 2018

View reviewed changes

chenlijun99 force-pushed the fix-issue-11108 branch from 7a7bc3d to c61d361 Compare March 15, 2018 14:29

Splaktar modified the milestones: 1.1.13, 1.1.14 Feb 10, 2019

Splaktar modified the milestones: 1.1.14, 1.1.15 Mar 14, 2019

Splaktar modified the milestones: 1.1.15, 1.1.16, 1.1.18, 1.1.19 Mar 29, 2019

Splaktar removed Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. pr: presubmit-failures labels May 18, 2019

Splaktar modified the milestones: 1.1.19, 1.1.21 May 30, 2019

Splaktar assigned mmalerba and unassigned josephperrott Jun 13, 2019

Splaktar unassigned mmalerba Jun 20, 2019

Splaktar modified the milestones: 1.1.21, 1.1.22 Aug 15, 2019

Splaktar modified the milestones: 1.1.22, 1.1.23 Oct 22, 2019

Splaktar modified the milestones: 1.1.23, 1.1.22 Mar 4, 2020

Splaktar requested a review from jelbourn March 4, 2020 19:14

jelbourn approved these changes Mar 5, 2020

View reviewed changes

Splaktar merged commit ecd55d0 into angular:master Mar 6, 2020

Splaktar added a commit that referenced this pull request Mar 6, 2020

chore: fix lint issue caused by PR #11156

abeb35a

Splaktar mentioned this pull request Mar 6, 2020

chore: fix lint issue caused by PR #11156 #11868

Merged

3 tasks

Splaktar added a commit that referenced this pull request Mar 6, 2020

chore: fix lint issue caused by PR #11156

2b0df7e

Splaktar mentioned this pull request Nov 9, 2020

select: trackBy - preset object, with property not in matching options, makes select dirty #10925

Closed

fix(select): when using trackBy, trigger ng-change only when tracked property is different #11156

fix(select): when using trackBy, trigger ng-change only when tracked property is different #11156

Uh oh!

Conversation

chenlijun99 commented Mar 10, 2018 • edited by Splaktar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chenlijun99 commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Splaktar commented Mar 13, 2018

Uh oh!

Splaktar commented Mar 13, 2018

Uh oh!

chenlijun99 commented Mar 13, 2018

Uh oh!

Splaktar commented Mar 13, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Mar 13, 2018

Uh oh!

chenlijun99 commented Mar 14, 2018

Uh oh!

Splaktar commented Mar 14, 2018

Uh oh!

Uh oh!

gkalpak Mar 15, 2018

Choose a reason for hiding this comment

Uh oh!

chenlijun99 Mar 15, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Splaktar commented May 18, 2019

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chenlijun99 commented Mar 10, 2018 •

edited by Splaktar

Loading

chenlijun99 commented Mar 13, 2018 •

edited

Loading