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

feat(chips): initial commit #2030

Closed
wants to merge 1 commit into from
Closed

feat(chips): initial commit #2030

wants to merge 1 commit into from

Conversation

typotter
Copy link
Contributor

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.

@@ -0,0 +1,39 @@
$chip-bgcolor: #F5F5F5;
$chip-border: 1px solid #DCDCDC;
Copy link
Member

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

@typotter
Copy link
Contributor Author

Thanks for the review, Jeremy. ptal

@@ -0,0 +1,51 @@
$chip-font-size: 13px;
Copy link
Member

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.

@ThomasBurleson
Copy link
Contributor

@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) {
Copy link
Contributor

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 )

Copy link
Member

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.

Copy link
Contributor

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.

@ThomasBurleson
Copy link
Contributor

@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.
@typotter
Copy link
Contributor Author

Comments addressed and/or rambled on in response. rebased, squashed, and pushed. Please, take another look :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants