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

bonniezhou · 2018-02-05T23:12:26Z

Resolves #2005.

Adjust styles (mostly padding) to handle icons in chips. This PR does not attach JS functionality to any icons yet.

codecov-io · 2018-02-05T23:27:11Z

Codecov Report

Merging #2191 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2191   +/-   ##
=======================================
  Coverage   98.72%   98.72%           
=======================================
  Files          93       93           
  Lines        3857     3857           
  Branches      501      501           
=======================================
  Hits         3808     3808           
  Misses         49       49

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cc5dd5...2a4f687. Read the comment docs.

kfranqueiro · 2018-02-13T18:28:53Z

packages/mdc-chips/README.md

@@ -52,13 +52,39 @@ npm install --save @material/chips
 </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 the relevant class (either `mdc-chip--with-leading-icon` or `mdc-chip--with-trailing-icon`) to the chip element, add an `i` element with your preferred icon, and give it a class of `mdc-chip__icon`. If you're adding a trailing icon, also give the icon a class of `mdc-chip__icon--trailing`.


The CSS styles below seem to suggest that trailing icons can be individually hovered/focused. If so, we might want to consider using a button element (or barring that, an a element which we'd cancel click events on) for trailing icon rather than an i element, if clicking the trailing icon has a distinct action from clicking the rest of the chip. Otherwise you're never actually going to trigger the :focus style below without also going out of your way to specify tabindex.

On the other hand, if clicking the trailing icon does the same thing as clicking the rest of the chip, I'm not sure whether the icon itself should be individually focusable at all? We should determine which element[s] should be focusable and consider updating our DOM accordingly. Right now, nothing on the chips demo page is focusable.

Okay, I just looked at this again after seeing Andy's comments, and realized things on the demo page are focusable... but it's almost imperceptible that things are getting focused unless you know exactly where you should be looking. I think we may have a design problem...

cc @acdvorak on this since he had some reservations about my button element idea in particular (mainly related to needing to override more styles, and maybe a concern about issues with padding in some browsers?)

Good spotting, I'll ask designers about the imperceptible focus issue.

I think it's fine to specify tabindex for icons, that's what we do for text field too. Using a i element makes the most sense semantically and specifying tabindex is a small price to pay.

We also need to specify tabindex for each chip, which leads to the question of whether we should use a button element for a chip. But from a design/semantics standpoint we probably want to separate the concept of a button from the concept of a chip as much as possible and not confuse clients...

I agree, chips are not buttons.

If anything, they're closer to checkboxes or radio buttons, but we can't really use those as the base element, so <div tabindex="0"> or <span tabindex="0"> seem reasonable to me.

Semantically, the "X" icon is a button, but it seems weird to use a <button> element for that, so I would also vote to stick with <i tabindex="0" role="button">.

It's true that we do have other components doing the same thing, and it's true that using the i element for icons is pretty pervasive nowadays, so yeah, I guess this discussion can be tabled.

The upside of a more semantically-correct element is not only getting tabindex for free but also potentially ARIA role (and technically we should probably have both?). However, it might also be the case that different roles make sense for different types of chips/icons, which would also discourage settling on a specific more semantic element. (I know Andy brought this up on the choice chips PR.)

FWIW the i element itself is arguably not semantic at all; it originally indicated italicized text back in the 90s, and was later discouraged in lieu of other tags like em that do have semantic meaning.

From MDN:

The <i> represents a range of text that is set off from the normal text for some reason, for example, technical terms, foreign language phrases, or fictional character thoughts. It is typically displayed in italic type.

Use this element only when there is not a more appropriate semantic element.

kfranqueiro · 2018-02-13T18:39:00Z

packages/mdc-chips/chip/mdc-chip.scss

+  outline: none;
+}
+
+.mdc-chip__icon.mdc-chip__icon--trailing {


I don't think most of these rules need the double class selector here?

We might want to use 2 classes for font-size specifically though, given the snag Brendan ran into when he can't guarantee ordering of material-icons and MDC stylesheets.

Based on a conversation I just had with Brendan, keep the width/height/font-size with the 2-class selector... the hover and focus are probably safe to just have .mdc-chip__icon--trailing though.

Why do we need both classes? All of the chips CSS rules are in a single file, so CSS load order shouldn't be a problem. What am I missing?

material-icons is not in the same file, or even the same repository. See #2242.

Gotcha. Can we add a comment above each 2-class-selector explaining why we need the extra class?

kfranqueiro · 2018-02-13T18:40:25Z

packages/mdc-chips/chip/mdc-chip.scss

@@ -33,9 +33,41 @@
  overflow: hidden;
 }

+.mdc-chip--with-leading-icon {


Is it not possible to accomplish this via padding or margin on the icons themselves, rather than needing to add one or two classes to every chip in addition to adding the icon?

Ohh so negative margins are a thing. Will do!

kfranqueiro · 2018-02-13T18:41:16Z

packages/mdc-chips/_variables.scss

+$mdc-chip-icon-color: rgba(0, 0, 0, .54);
+$mdc-chip-trailing-icon-size: 18px;
+$mdc-chip-trailing-icon-hover: rgba(0, 0, 0, .62);
+$mdc-chip-trailing-icon-focus:rgba(0, 0, 0, .87);


Missing space after : here. Also, add -color to the hover/focus variable names?

acdvorak · 2018-02-13T18:38:50Z

packages/mdc-chips/README.md

+`mdc-chip--with-leading-icon` | Optional. Indicates the chip contains a leading icon
+`mdc-chip--with-trailing-icon` | Optional. Indicates the chip contains a trailing icon
+`mdc-chip__text` | Mandatory. Indicates the text content of the chip
+`mdc-chip__icon` | Optional. Indicates a leading or trailing icon in the chip


Can we mention that "leading" is the default unless --trailing is used?

Updated it so either --leading or --trailing is mandatory, which also removes the need for mdc-chip--with-leading-icon and mdc-chip--with-trailing-icon.

acdvorak · 2018-02-13T18:39:51Z

packages/mdc-chips/_variables.scss

+$mdc-chip-icon-color: rgba(0, 0, 0, .54);
+$mdc-chip-trailing-icon-size: 18px;
+$mdc-chip-trailing-icon-hover: rgba(0, 0, 0, .62);
+$mdc-chip-trailing-icon-focus:rgba(0, 0, 0, .87);


Nit: Add a space after :

acdvorak · 2018-02-13T18:45:04Z

packages/mdc-chips/chip/mdc-chip.scss

@@ -33,9 +33,41 @@
  overflow: hidden;
 }


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.

Done, thanks for the tip!

acdvorak · 2018-02-13T18:47:37Z

packages/mdc-chips/chip/mdc-chip.scss

+  color: $mdc-chip-icon-color;
+  vertical-align: middle;
+  margin: 0 4px;
+  border-radius: 16px;


border-radius: 50% should do the trick, and it avoids hard-coding a specific pixel value 😀

acdvorak · 2018-02-13T18:53:30Z

packages/mdc-chips/_variables.scss

@@ -19,3 +19,9 @@ $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-with-icon-padding: 4px;
+
+$mdc-chip-icon-color: rgba(0, 0, 0, .54);


I normally prefer to use the global theme variables when possible (e.g., text-primary-on-background), because it makes it much easier for clients to create a dark theme.

In this case, though, some of the other values in this file don't map directly to theme vars (e.g., .08, .62), so it might not be feasible.

Just something to keep in mind in the future 🙂

Replaced mdc-chip-ink-color-default with text-primary-on-light! I kept the icon colors though since the global variables are supposed to be for text.

acdvorak · 2018-02-13T18:55:18Z

demos/chips.html

-          <div class="mdc-chip">
-            <div class="mdc-chip__text">Add to Calendar</div>
+          <div class="demo-chip mdc-chip mdc-chip--with-leading-icon mdc-chip--with-trailing-icon">
+            <i class="material-icons mdc-chip__icon" tabindex="0">face</i>


If we're going to have tabindex="0" here, we should also add :focus and :hover styles for leading icons.

Removed tabindex since interacting with the leading icon should be the same as interacting with the chip.

acdvorak · 2018-02-13T18:56:09Z

demos/chips.html

+          <div class="demo-chip mdc-chip mdc-chip--with-leading-icon mdc-chip--with-trailing-icon">
+            <i class="material-icons mdc-chip__icon" tabindex="0">face</i>
+            <div class="mdc-chip__text">Jane Smith</div>
+            <i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0">more_vert</i>


Add ARIA and title attributes: role="button" title="More options"

Ditto for other elements that have a tabindex="0"

Should chips themselves also have role="button"? I guess this is related to Ken's comment above.

I suspect that once all the variants of chips are in, we should document guidance on what roles make sense for what types of chips.

acdvorak · 2018-02-13T18:57:33Z

packages/mdc-chips/chip/mdc-chip.scss

+  outline: none;
+}
+
+.mdc-chip__icon.mdc-chip__icon--trailing {


Can we simplify this selector to just .mdc-chip__icon--trailing?

kfranqueiro

FYI I realized that currently interacting with the trailing icon still causes ripple, which probably isn't wanted (check with designer to be sure) but we will probably only be able to prevent when we add JS.

kfranqueiro · 2018-02-15T18:13:01Z

demos/chips.scss

@@ -17,6 +17,10 @@
 @import "./common";
 @import "../packages/mdc-chips/mdc-chips";

+.demo-chip {


I am wondering whether we should be building in at least the left/right margins if not also even top/bottom, since chips are intended to be in sets and I assume we have an opinion on how chips within a set are spaced apart? (Even if we make that style specific to chip-set...)

+1, let's give each chip a baseline margin.

However, we should clarify with design whether the margins should also push the chips Npx away from the bounding box of their parent container (i.e., the chip set).

E.g.:

vs.:

(the grey borders are only to show the boundaries of the .mdc-chip-set element)

It makes a difference when there's a heading, or input, or some other element nearby that needs to be horizontally aligned.

I also strongly suggest that we change .mdc-chip-set to use display: flex; flex-wrap: wrap; to collapse the whitespace in the markup, because it will give us much better control over spacing (so we can match the spec).

Design confirmed that the margins should also push the chips away from the chip-set border.
I've hard-coded a baseline margin on the chips for now and filed #2265 for a separate PR.

kfranqueiro · 2018-02-15T18:14:38Z

demos/chips.html

@@ -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">


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

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.

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).

kfranqueiro · 2018-02-15T18:15:24Z

packages/mdc-chips/README.md

+```html
+<div class="mdc-chip mdc-chip--with-trailing-icon">
+  <div class="mdc-chip__text">Jane Smith</div>
+  <i class="material-icons mdc-chip__icon mdc-chip__icon--trailing" tabindex="0">cancel</i>


Add role="button" to the example

kfranqueiro · 2018-02-15T18:16:28Z

packages/mdc-chips/README.md

+##### Leading icon
+
+```html
+<div class="mdc-chip mdc-chip--with-leading-icon">


Remove --with-leading-icon here and --with-trailing-icon below since they were re-engineered out of existence

kfranqueiro · 2018-02-15T18:17:25Z

packages/mdc-chips/chip/mdc-chip.scss

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


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

kfranqueiro · 2018-02-15T18:18:38Z

packages/mdc-chips/chip/mdc-chip.scss

+  }
+}
+
+.mdc-chip__icon.mdc-chip__icon--trailing {


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)

acdvorak

A few nits, otherwise LGTM!

acdvorak · 2018-02-15T01:24:30Z

packages/mdc-chips/README.md

+`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.


Yay! This seems much more intuitive to me 😀

acdvorak · 2018-02-15T01:25:50Z

packages/mdc-chips/chip/mdc-chip.scss

+}
+
+.mdc-chip__icon--leading {
+  margin: -4px 4px -4px -4px;


Awwwww yeeeeeeaaaaaa

acdvorak · 2018-02-15T01:34:01Z

packages/mdc-chips/chip/mdc-chip.scss

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


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 🙁

acdvorak · 2018-02-15T01:36:53Z

packages/mdc-chips/chip/mdc-chip.scss

+  }
+}
+
+.mdc-chip__icon.mdc-chip__icon--trailing {


Can we add a comment to the effect of:

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

bonniezhou · 2018-02-16T19:24:25Z

@kfranqueiro Confirmed with design that clicking the trailing icon should not trigger a ripple (filed #2266).

bonniezhou · 2018-02-16T19:28:27Z

@kfranqueiro @acdvorak Ready for another look! To summarize, nits are fixed and I filed #2259, #2265 and #2266 to be done in smaller PRs.

acdvorak

LGTM once my final 3 comments are addressed!

acdvorak · 2018-02-16T20:26:54Z

packages/mdc-chips/README.md

@@ -59,7 +59,7 @@ You can optionally add a leading icon (i.e. thumbnail) and/or a trailing icon to
 ##### Leading icon

 ```html
-<div class="mdc-chip mdc-chip--with-leading-icon">
+<div class="mdc-chip mdc-chip">


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

acdvorak · 2018-02-16T20:26:58Z

packages/mdc-chips/README.md

@@ -68,9 +68,9 @@ You can optionally add a leading icon (i.e. thumbnail) and/or a trailing icon to
 ##### Trailing icon

 ```html
-<div class="mdc-chip mdc-chip--with-trailing-icon">
+<div class="mdc-chip mdc-chip">


acdvorak · 2018-02-16T20:40:40Z

packages/mdc-chips/README.md

 ### CSS Classes

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


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.

I put it in #2265 since it requires the creation of a new file (mdc-chip-set/mdc-chip-set.scss)

bonniezhou and others added 5 commits February 5, 2018 14:12

feat(chips): Handle leading/trailing icon styles

5050824

Move styles to variables

22d0394

Update README

6f34c1d

Reorder css classes in readme

5c3365a

Merge branch 'master' into feat/chip/icon

a57a437

Merge branch 'master' into feat/chip/icon

9a3b946

YePpHa mentioned this pull request Feb 8, 2018

feat(Chips): implement chips prateekbh/preact-material-components#589

Merged

kfranqueiro suggested changes Feb 13, 2018

View reviewed changes

kfranqueiro reviewed Feb 13, 2018

View reviewed changes

acdvorak self-assigned this Feb 13, 2018

kfranqueiro reviewed Feb 13, 2018

View reviewed changes

kfranqueiro mentioned this pull request Feb 13, 2018

feat(chips): Add mdc-chip-set--choice variant #2215

Merged

acdvorak suggested changes Feb 13, 2018

View reviewed changes

bonniezhou added 10 commits February 13, 2018 16:54

Fix tabindex on chips and icons

152a9d5

despecify icon hover/focus styles

b2fc5b4

Make icons tabbable

27165d9

use global theme vars

52a8254

change 000 to black

28205ee

Use display inline-flex

7a411ef

border radius 50p

33fa00d

Move external margins to demo

72dbc4c

Add button role to icons and fix up demo html

23d3930

Update README

93fc133

kfranqueiro suggested changes Feb 15, 2018

View reviewed changes

acdvorak suggested changes Feb 15, 2018

View reviewed changes

bonniezhou and others added 3 commits February 16, 2018 11:01

hard code chip margins

a3e4d59

Nits

88fce91

Merge branch 'master' into feat/chip/icon

0f91516

acdvorak approved these changes Feb 16, 2018

View reviewed changes

bonniezhou added 2 commits February 16, 2018 13:50

Add position: relative; to mdc-chip for edge/ie

200638f

remove extra mdc-chip

2a4f687

kfranqueiro approved these changes Feb 16, 2018

View reviewed changes

bonniezhou merged commit be71f9f into master Feb 16, 2018

bonniezhou deleted the feat/chip/icon branch February 17, 2018 00:00

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

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

Conversation

bonniezhou commented Feb 5, 2018

codecov-io commented Feb 5, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdvorak Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdvorak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonniezhou commented Feb 16, 2018

bonniezhou commented Feb 16, 2018

acdvorak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 5, 2018 •

edited

Loading

acdvorak Feb 15, 2018 •

edited

Loading