-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Conversation
@@ -0,0 +1,39 @@ | |||
$chip-bgcolor: #F5F5F5; | |||
$chip-border: 1px solid #DCDCDC; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These colors should almost certainly be in terms of the theme
Thanks for the review, Jeremy. ptal |
@@ -0,0 +1,51 @@ | |||
$chip-font-size: 13px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add !default to each of these so that someone could theoretically override them.
@typotter - Can you rebase this PR from latest master source and squash your commits? Then I can finish reviewing and merge. Thx. |
this.appendChipBuffer(); | ||
break; | ||
case this.$mdConstant.KEY_CODE.BACKSPACE: // backspace | ||
if (!this.chipBuffer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use if ( this.chipBuffer.length )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasBurleson why? The truthy check semantically says "Do I have no value?", which is appropriate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jelbourn - The value chipBuffer is initialized to '' and then cleared to ''. So for consistency, I thought a valid string check was appropriate.
@typotter - With the exception of my questions, very cool looking. 👍 |
Supports basic list mutation (add, remove items). Does not yet support child-input elements (renders its own input element by default). TODO/future work laid out in directive comments.
Comments addressed and/or rambled on in response. rebased, squashed, and pushed. Please, take another look :) |
Supports basic list mutation (add, remove items). Does not yet support
child-input elements (renders its own input element by default).
TODO/future work laid out in directive comments.