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): Handle leading/trailing icon styles #2191

Merged
merged 21 commits into from
Feb 16, 2018
Merged

feat(chips): Handle leading/trailing icon styles #2191

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5050824
feat(chips): Handle leading/trailing icon styles
bonniezhou Feb 5, 2018
22d0394
Move styles to variables
bonniezhou Feb 5, 2018
6f34c1d
Update README
bonniezhou Feb 5, 2018
5c3365a
Reorder css classes in readme
bonniezhou Feb 5, 2018
a57a437
Merge branch 'master' into feat/chip/icon
bonniezhou Feb 5, 2018
9a3b946
Merge branch 'master' into feat/chip/icon
bonniezhou Feb 7, 2018
152a9d5
Fix tabindex on chips and icons
bonniezhou Feb 14, 2018
b2fc5b4
despecify icon hover/focus styles
bonniezhou Feb 14, 2018
27165d9
Make icons tabbable
bonniezhou Feb 14, 2018
52a8254
use global theme vars
bonniezhou Feb 14, 2018
28205ee
change 000 to black
bonniezhou Feb 14, 2018
7a411ef
Use display inline-flex
bonniezhou Feb 14, 2018
33fa00d
border radius 50p
bonniezhou Feb 14, 2018
72dbc4c
Move external margins to demo
bonniezhou Feb 14, 2018
23d3930
Add button role to icons and fix up demo html
bonniezhou Feb 14, 2018
93fc133
Update README
bonniezhou Feb 14, 2018
a3e4d59
hard code chip margins
bonniezhou Feb 16, 2018
88fce91
Nits
bonniezhou Feb 16, 2018
0f91516
Merge branch 'master' into feat/chip/icon
bonniezhou Feb 16, 2018
200638f
Add position: relative; to mdc-chip for edge/ie
bonniezhou Feb 16, 2018
2a4f687
remove extra mdc-chip
bonniezhou Feb 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 83 additions & 19 deletions demos/chips.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,65 +38,129 @@
<main class="mdc-toolbar-fixed-adjust">
<section class="hero">
<div class="mdc-chip-set">
<div class="mdc-chip">
<div class="demo-chip mdc-chip" tabindex="0">
Copy link
Contributor

Choose a reason for hiding this comment

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

We've given the trailing icons role, but what about chips themselves? (I forget if that's being handled in #2215)

Copy link
Contributor

@acdvorak acdvorak Feb 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think @bonniezhou decided to do most of the a11y stuff (or the tricky parts that require JS, anyway) in a separate follow up PR, cuz shoehorning it into this one was getting kinda hairy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's tracked in #2259. Also, design confirmed that we should add a role but looking into options other than button (maybe menuitem or something similar).

<div class="mdc-chip__text">Chip One</div>
</div>
<div class="mdc-chip">
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Chip Two</div>
</div>
<div class="mdc-chip">
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Chip Three</div>
</div>
<div class="mdc-chip">
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Chip Four</div>
</div>
</div>
</section>

<section class="example">
<h2>Entry Chips</h2>
<div class="mdc-chip-set">
<div class="mdc-chip">
<div class="mdc-chip__text">Add to Calendar</div>
<div class="demo-chip mdc-chip" tabindex="0">
<i class="material-icons mdc-chip__icon mdc-chip__icon--leading">face</i>
<div class="mdc-chip__text">Jane Smith</div>
<i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0" role="button" title="More options">more_vert</i>
</div>
<div class="demo-chip mdc-chip" tabindex="0">
<i class="material-icons mdc-chip__icon mdc-chip__icon--leading">face</i>
<div class="mdc-chip__text">John Doe</div>
<i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0" role="button" title="More options">more_vert</i>
</div>
</div>
</section>

<section class="example">
<h2>Choice Chips</h2>
<div class="mdc-chip-set">
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Extra Small</div>
</div>
<div class="mdc-chip">
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Small</div>
</div>
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Medium</div>
</div>
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Large</div>
</div>
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Extra Large</div>
</div>
</div>
</section>

<section class="example">
<h2>Filter Chips</h2>
<div class="mdc-chip-set">
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Tops</div>
<i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0" role="button">cancel</i>
</div>
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Bottoms</div>
<i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0" role="button">cancel</i>
</div>
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Shoes</div>
<i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0" role="button">cancel</i>
</div>
<div class="demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Accessories</div>
<i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0" role="button">cancel</i>
</div>
</div>
</section>

<section class="example">
<h2>Action Chips</h2>
<div class="mdc-chip-set">
<div class="demo-chip mdc-chip" tabindex="0">
<i class="material-icons mdc-chip__icon mdc-chip__icon--leading">wb_sunny</i>
<div class="mdc-chip__text">Turn on lights</div>
</div>
<div class="demo-chip mdc-chip" tabindex="0">
<i class="material-icons mdc-chip__icon mdc-chip__icon--leading">bookmark</i>
<div class="mdc-chip__text">Bookmark</div>
</div>
<div class="mdc-chip">
<div class="mdc-chip__text">Set Alarm</div>
<div class="demo-chip mdc-chip" tabindex="0">
<i class="material-icons mdc-chip__icon mdc-chip__icon--leading">alarm</i>
<div class="mdc-chip__text">Set alarm</div>
</div>
<div class="mdc-chip">
<div class="mdc-chip__text">Get Directions</div>
<div class="demo-chip mdc-chip" tabindex="0">
<i class="material-icons mdc-chip__icon mdc-chip__icon--leading">directions</i>
<div class="mdc-chip__text">Get directions</div>
</div>
</div>
</section>

<section class="example">
<h2>Custom theme</h2>
<div class="mdc-chip-set">
<div class="custom-chip-primary mdc-chip">
<div class="custom-chip-primary demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Add to Calendar</div>
</div>
<div class="custom-chip-primary mdc-chip">
<div class="custom-chip-primary demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Bookmark</div>
</div>
<div class="custom-chip-primary mdc-chip">
<div class="custom-chip-primary demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Set Alarm</div>
</div>
<div class="custom-chip-primary mdc-chip">
<div class="custom-chip-primary demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Get Directions</div>
</div>
</div>
<div class="mdc-chip-set">
<div class="custom-chip-secondary mdc-chip">
<div class="custom-chip-secondary demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Add to Calendar</div>
</div>
<div class="custom-chip-secondary mdc-chip">
<div class="custom-chip-secondary demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Bookmark</div>
</div>
<div class="custom-chip-secondary mdc-chip">
<div class="custom-chip-secondary demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Set Alarm</div>
</div>
<div class="custom-chip-secondary mdc-chip">
<div class="custom-chip-secondary demo-chip mdc-chip" tabindex="0">
<div class="mdc-chip__text">Get Directions</div>
</div>
</div>
Expand Down
37 changes: 32 additions & 5 deletions packages/mdc-chips/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,25 +40,52 @@ npm install --save @material/chips

```html
<div class="mdc-chip-set">
<div class="mdc-chip">
<div class="mdc-chip" tabindex="0">
<div class="mdc-chip__text">Chip content</div>
</div>
<div class="mdc-chip">
<div class="mdc-chip" tabindex="0">
<div class="mdc-chip__text">Chip content</div>
</div>
<div class="mdc-chip">
<div class="mdc-chip" tabindex="0">
<div class="mdc-chip__text">Chip content</div>
</div>
</div>
```

#### Leading and Trailing Icons

You can optionally add a leading icon (i.e. thumbnail) and/or a trailing icon to a chip. To add an icon, add an `i` element with your preferred icon, give it a class of `mdc-chip__icon`, and a class of either `mdc-chip__icon--leading` or `mdc-chip__icon--trailing`. If you're adding a trailing icon, also set `tabindex="0"` and `role="button"` to make it accessible by keyboard and screenreader.

##### Leading icon

```html
<div class="mdc-chip mdc-chip">
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like chips too, but 2 is probably too much here 😉

<i class="material-icons mdc-chip__icon mdc-chip__icon--leading">event</i>
<div class="mdc-chip__text">Add to calendar</div>
</div>
```

##### Trailing icon

```html
<div class="mdc-chip mdc-chip">
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

<div class="mdc-chip__text">Jane Smith</div>
<i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0" role="button">cancel</i>
</div>
```

### CSS Classes

CSS Class | Description
--- | ---
`mdc-chip-set` | Mandatory. Indicates the set that the chip belongs to
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change .mdc-chip-set to use display: flex; flex-wrap: wrap to collapse the whitespace in the markup?

It will give us much better control over spacing, so we can match the spec exactly.

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 put it in #2265 since it requires the creation of a new file (mdc-chip-set/mdc-chip-set.scss)

`mdc-chip` | Mandatory.
`mdc-chip__text` | Mandatory. Indicates the text content of the chip.
`mdc-chip-set` | Mandatory. Indicates the set that the chip belongs to.
`mdc-chip__text` | Mandatory. Indicates the text content of the chip
`mdc-chip__icon` | Optional. Indicates an icon in the chip
`mdc-chip__icon--leading` | Optional. Indicates a leading icon in the chip
`mdc-chip__icon--trailing` | Optional. Indicates a trailing icon in the chip

> _NOTE_: Every element that has an `mdc-chip__icon` class must also have either the `mdc-chip__icon--leading` or `mdc-chip__icon--trailing` class.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay! This seems much more intuitive to me 😀


### Sass Mixins

Expand Down
6 changes: 5 additions & 1 deletion packages/mdc-chips/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

$mdc-chip-border-radius-default: 16px;
$mdc-chip-fill-color-default: rgba(black, .08);
$mdc-chip-ink-color-default: rgba(black, .87);
$mdc-chip-horizontal-padding: 12px;
$mdc-chip-vertical-padding: 7px;

$mdc-chip-icon-color: rgba(black, .54);
$mdc-chip-trailing-icon-size: 18px;
$mdc-chip-trailing-icon-hover-color: rgba(black, .62);
$mdc-chip-trailing-icon-focus-color: rgba(black, .87);
42 changes: 37 additions & 5 deletions packages/mdc-chips/chip/mdc-chip.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,50 @@
@include mdc-ripple-radius-bounded;
@include mdc-chip-corner-radius($mdc-chip-border-radius-default);
@include mdc-chip-fill-color($mdc-chip-fill-color-default);
@include mdc-chip-ink-color($mdc-chip-ink-color-default);
@include mdc-chip-ink-color(text-primary-on-light);

display: inline-block;
margin: 5px;
display: inline-flex;
align-items: center;
margin: 4px;
padding: $mdc-chip-vertical-padding $mdc-chip-horizontal-padding;
cursor: pointer;
outline: none;
overflow: hidden;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest using display: inline-flex on .mdc-chip to collapse whitespace in the markup. This will give you more control over spacing.

Then, instead of vertical-align: middle on the child elements, you can just set align-items: center on .mdc-chip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for the tip!


.mdc-chip__text {
display: inline-block;
vertical-align: middle;
// Set line-height to be <18px to force the content height of mdc-chip to be 18px.
line-height: 17px;
}

.mdc-chip__icon {
color: $mdc-chip-icon-color;
vertical-align: middle;
border-radius: 50%;
outline: none;
}

.mdc-chip__icon--leading {
margin: -4px 4px -4px -4px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Awwwww yeeeeeeaaaaaa

}

.mdc-chip__icon--trailing {
margin: 0 -4px 0 4px;

&:hover {
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 normally put a blank line before &... selectors
(unless they're the first thing in a curly block {}).

I was going to suggest adding a stylelint setting to catch this in the future, but I can't seem to find one for it 🙁

Copy link
Contributor

Choose a reason for hiding this comment

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

Newline before this block? (Kinda surprised lint doesn't complain?)

color: $mdc-chip-trailing-icon-hover-color;
}

&:focus {
color: $mdc-chip-trailing-icon-focus-color;
}
}

// This overrides styles defined in the .material-icons CSS class
// which is loaded separately, so the order of CSS definitions is not
// guaranteed. Therefore, increase specifity to ensure overrides apply.
.mdc-chip__icon.mdc-chip__icon--trailing {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment to the effect of:

// Higher specificity needed to ensure that these values
// always take precedence over `.material-icons`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment indicating these are more specific to ensure they override icon styles in other libraries e.g. material-icons (like in https://github.com/material-components/material-components-web/pull/2242/files)

width: $mdc-chip-trailing-icon-size;
height: $mdc-chip-trailing-icon-size;
font-size: $mdc-chip-trailing-icon-size;
}