Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix secondary placeholder problem in chips - #3804

Closed
dbwhddn10 wants to merge 2 commits into
angular:masterfrom
dbwhddn10:patch-3
Closed

fix secondary placeholder problem in chips#3804
dbwhddn10 wants to merge 2 commits into
angular:masterfrom
dbwhddn10:patch-3

Conversation

@dbwhddn10

Copy link
Copy Markdown
Contributor

No description provided.

@dbwhddn10dbwhddn10 changed the title Update chipsController.jsfix secondary placeholder problem in chipsJul 18, 2015
@ThomasBurlesonThomasBurleson modified the milestone: 0.12.0Aug 20, 2015
@ThomasBurlesonThomasBurleson modified the milestones: 1.0-rc1, 1.0-rc2Oct 27, 2015
@topherfangiotopherfangio added needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community needs: feedback The issue creator or community need to respond to questions in this issue labels Dec 30, 2015
@topherfangio

Copy link
Copy Markdown
Contributor

I need to dig into this a bit further before we can review it.

@dbwhddn10 Can you provide any additional info as to what this fixes? Ideally a description, test case and an issue # would be fantastic!

@ericgundrum

Copy link
Copy Markdown
Contributor

@topherfangio, I have unit tests demonstrating issues #2770 and #4476. (Currently there are no unit tests for the placeholder feature of mdChips input.) I've created PR #6535 with those tests and the related fix. (The fix is a single line.)

I wrote a test to match the issue PR a5dba2a is trying to fix. However, there is much other mdChips code that fails to check if the ngModel is defined. There seems to be an inherent assumption that ngModel is defined or that catastrophic failure is preferred when it isn't. It may be better to separate this issue and PR from issues #2770 and #4476.

@EladBezalel

Copy link
Copy Markdown
Member

@ericgundrum thanks.
@dbwhddn10 can you please rebase?
@topherfangio can you please investigate? :)

@topherfangio

Copy link
Copy Markdown
Contributor

I'm going to go ahead and close this in favor of #6175 since it does the NPE check only and #6535 has already been merged.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs: feedbackThe issue creator or community need to respond to questions in this issueneeds: investigationThe cause of this issue is not well understood and needs to be investigated by the team or community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@dbwhddn10@topherfangio@ericgundrum@EladBezalel@ThomasBurleson