Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

fix(select): fix set form to pristine if ng-model for multiple select is predefined. #6782

Closed

Conversation

devversion
Copy link
Member

Bug introduced in 09bd5a3

Fixes #6556

@@ -228,9 +228,10 @@ function SelectDirective($mdSelect, $mdUtil, $mdTheming, $mdAria, $compile, $par
});
}

if (formCtrl) {
if (formCtrl && angular.isDefined(attr.multiple)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So if we are not using multiple, then we never $setPristine()? Is that valid logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that is valid, as you can see in 09bd5a3, we should only set the form to pristine if multiple objects are pre-selected in the ngModel.

I manually tested that too, if there is a predefined value for a single select, we don't need to set the form pristine on initialization.

@ThomasBurleson
Copy link
Contributor

@devversion - for 99% of all PRs, each one will need 1 or more unit tests. Can you provide a unit test to validate and verify these changes?

@devversion
Copy link
Member Author

@ThomasBurleson - Yes, I will add a Unit Test for both cases.

@ThomasBurleson ThomasBurleson added the needs: unit tests This PR needs unit tests to cover the changes being proposed label Jan 26, 2016
@ThomasBurleson ThomasBurleson self-assigned this Jan 26, 2016
@devversion
Copy link
Member Author

@ThomasBurleson - Added two tests for the $pristine behavior. also when not using multiple

@ThomasBurleson ThomasBurleson added needs: review This PR is waiting on review from the team and removed needs: unit tests This PR needs unit tests to cover the changes being proposed labels Jan 30, 2016
@ThomasBurleson ThomasBurleson added this to the 1.0.5 milestone Jan 30, 2016
@ThomasBurleson ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Jan 30, 2016
ThomasBurleson pushed a commit that referenced this pull request Jan 31, 2016
ErinCoughlan pushed a commit to ErinCoughlan/material that referenced this pull request Feb 9, 2016
@devversion devversion deleted the fix/select-multiple-pristine branch April 21, 2016 11:59
@Splaktar Splaktar modified the milestones: 1.0.5, 1.1.0 Oct 25, 2018
@angular angular locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
- Breaking Change pr: merge ready This PR is ready for a caretaker to review type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants