fix(md-chips placeholder): correct placeholder/secondary logic #6535
Conversation
Add tests for the placeholder feature of `chipsController`(<mdChips>). Previously there were no tests. These tests demonstrate the failures reported as github issues angular#2770 and angular#4476.
The logic test deciding when to show the secondary placeholder was backwards, causing the secondary placeholder to be shown when no chips were present. Fixes github issues angular#2770 and angular#4476.
ericgundrum
changed the title
|
Please squash your commits, so it can be cherry picked easily. |
|
@devversion, are you sure that is preferable? My point with two commits was for one to demonstrate the failure of the existing code and then a second to show a fix. By keeping them separate either one can be taken independent of the other -- they do not depend upon each other. |
|
As contributors we should always squash our commits to one commit, so Thomas Burleson, the main PR Reviewer, can easily merge the PR. But you can wait with Squashing till the PR got reviewed. |
|
@ericgundrum Thanks for the PR. LGTM. In general, we do request that you squash all commits, and Thomas may ask that before he merges, but sometimes if there are very few commits, he will squash them himself. It just saves him a step as we always squash before merging. |
| @@ -183,7 +183,7 @@ MdChipsCtrl.prototype.getPlaceholder = function() { | |||
| // Allow `secondary-placeholder` to be blank. | |||
| var useSecondary = (this.items.length && | |||
| (this.secondaryPlaceholder == '' || this.secondaryPlaceholder)); | |||
| return useSecondary ? this.placeholder : this.secondaryPlaceholder; | |||
| return useSecondary ? this.secondaryPlaceholder : this.placeholder; | |||
There was a problem hiding this comment.
@topherfangio - this logic looks wrong. Can you check.
There was a problem hiding this comment.
Nope, our existing logic has been wrong the whole time. It currently says
if use secondary
use regular placeholder
else
use secondary placeholder
which is totally backwards. This fixes it. See #2770 for Tyler's comment about it being backwards.
The logic test deciding when to show the secondary placeholder was backwards, causing the secondary placeholder to be shown when no chips were present. Fixes github issues angular#2770 and angular#4476. Closes angular#6535
Test and then fix the md-chips placeholder logic so that the secondary placeholder text appears only when it is documented to appear. Addresses issues #2770 and #4476.
The first commit provides test code to demonstrate the failure. The second commit changes the implementation code to pass the tests.