Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(chips): Initial skeleton/demo. #1855

Merged
merged 1 commit into from
Nov 29, 2016
Merged

feat(chips): Initial skeleton/demo. #1855

merged 1 commit into from
Nov 29, 2016

Conversation

topherfangio
Copy link
Contributor

@topherfangio topherfangio commented Nov 14, 2016 •
edited
Loading

Create the initial Chips skeleton with a very basic working demo of static, styled chips.

screen shot 2016-11-17 at 12 03 26 am

References #120.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 14, 2016
@topherfangio
Copy link
Contributor Author

@jelbourn Sorry; I just remembered that we discussed changing this to md-chip-list. I will do that and resubmit.

@topherfangio topherfangio added in progress This issue is currently in progress and removed pr: needs review labels Nov 14, 2016
@topherfangio topherfangio added pr: needs review and removed in progress This issue is currently in progress labels Nov 15, 2016
jelbourn
jelbourn reviewed Nov 16, 2016
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Need skeleton for unit tests as well.

src/lib/chips/chip-list.ts

static COMPONENT_CONFIG: Component = {
moduleId: module.id,
selector: 'md-chip-list, [md-chip-list]',
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to do the attribute selector until we have a strong use-case for it.

src/lib/chips/chip-list.ts
'role': 'listbox',
},
queries: {
items: new ContentChildren(MdChip)
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this specifically (using new Something in this metadata) won't work in AoT compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will play with this in the AoT and see what happens, but I realized it's actually not used in the skeleton (I built the skeleton by deleting code from what I showed you before) so I will remove it from this PR and probably have it in my next one.

src/lib/chips/chip-list.ts
@Component(MdChipList.COMPONENT_CONFIG)
export class MdChipList {

static COMPONENT_CONFIG: Component = {
Copy link
Member

Choose a reason for hiding this comment

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

If this does work in AoT mode, then it should just be an exported constant, not a static member on the MdChipList

Either way, though, I'm tempted to say let's not worry about this for the initial version and add a TODO to explore the idea later.

src/lib/chips/chip-list.ts
constructor(private _elementRef: ElementRef) {}

ngAfterContentInit(): void {
this._elementRef.nativeElement.classList.add('md-chips');
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to add class to the host oject in the component metadata for this:

host: {
  'class': 'md-chip-list'
}

Same for chip below.

src/lib/chips/chip.ts
Component,
ElementRef,
Renderer
} from '@angular/core';
Copy link
Member

Choose a reason for hiding this comment

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

Only need to wrap like this if it's longer than 100 cols.

src/lib/chips/chips.scss
border-radius: $md-chip-horizontal-padding * 2;

background-color: #E0E0E0;
color: rgba(0, 0, 0, 0.87);
Copy link
Member

Choose a reason for hiding this comment

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

All colors should be in a _chips-theme.scss file. See the other components for an example.

src/lib/chips/chips.scss
background-color: #E0E0E0;
color: rgba(0, 0, 0, 0.87);
font-size: 13px;
line-height: 16px;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should be setting font-size and line-height. Could we use whatever value is current applied on this element?

@hansl thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would love some additional guidance on this. Those values were set based off of the spec, so if we are allowing users to deviate, it may change some other values (like the border raidus in particular).

Is there any plan to use em or rem instead of pixels?

@jelbourn
Copy link
Member

Also there's a lint error on CI.

@topherfangio topherfangio added in progress This issue is currently in progress and removed pr: needs review labels Nov 17, 2016
@topherfangio
feat(chips): Initial skeleton/demo.
fb22bbe 
Create the initial Chips skeleton with a very basic working demo
of static, styled chips.

References angular#120.
@topherfangio topherfangio added pr: needs review and removed in progress This issue is currently in progress labels Nov 17, 2016
@topherfangio
Copy link
Contributor Author

@jelbourn Ready for re-review. Made all requested changes except for removing font-size/line-height as I'm not sure what to replace it with. Would love some more feedback on that.

@jelbourn
Copy link
Member

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Nov 17, 2016
@jelbourn
Copy link
Member

@kara FYI this will need new build rules.

fxck
fxck reviewed Nov 25, 2016
View reviewed changes
src/lib/chips/chips.scss

.md-chip {
display: inline-block;
padding: $md-chip-vertical-padding $md-chip-horizontal-padding
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to repeat these,

padding: $md-chip-vertical-padding $md-chip-horizontal-padding;

is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed; will fix in my next PR.

@tinayuangao tinayuangao merged commit edf5cdf into angular:master Nov 29, 2016
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@fxck fxck fxck left review comments

Assignees

@jelbourn jelbourn

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@topherfangio @jelbourn @fxck @googlebot @tinayuangao