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.

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

fix(toolbar): Fix colors for svg icons. Update custom-toolbar demo #2331

Merged
merged 6 commits into from
Mar 7, 2018

Conversation

williamernest
Copy link
Contributor

Fixes: #1821
Adds fill: currentColor to the icons, and updates the custom-toolbar.html demo to show one icon using SVG.

@codecov-io
Copy link

codecov-io commented Mar 1, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2331   +/-   ##
=======================================
  Coverage   98.85%   98.85%           
=======================================
  Files         100      100           
  Lines        4118     4118           
  Branches      527      527           
=======================================
  Hits         4071     4071           
  Misses         47       47

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 dc52201...550ddee. Read the comment docs.

.demo-toolbar__menu-icon--custom,
.demo-toolbar__icon--custom {
@include mdc-toolbar-icon-ink-color($material-color-red-100);
.demo-toolbar__menu-icon--custom,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was broken due to css specificity. Our toolbar generates .mdc-toolbar .mdc-toolbar__icon, but this was producing just .demo-toolbar__icon--custom.

@@ -141,6 +141,14 @@ Icons can be added as anchor tags, `span`s, or `button`s to `mdc-toolbar`. There
`mdc-toolbar__menu-icon` represents the left most icon in `mdc-toolbar` usually to the left of `mdc-toolbar__title`.
`mdc-toolbar__icon` represents any icons placed on the right side of an `mdc-toolbar`.

When using `svg` icons, ensure you wrap the `svg` element in a `span` and include the `mdc-toolbar__icon` class.
Copy link
Contributor Author

@williamernest williamernest Mar 1, 2018

Choose a reason for hiding this comment

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

SVG's don't have a ::before or ::after element, which we need for the ripple, so I added this section to the Readme.

When using `svg` icons, ensure you wrap the `svg` element in a `span` and include the `mdc-toolbar__icon` class.

```html
<span class="mdc-toolbar__icon">
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Indentation:

```html
<a href="#" class="mdc-toolbar__icon" title="Add to favorites">
  <svg>...</svg>
</a>
```...

@@ -36,7 +36,9 @@
<section class="mdc-toolbar__section mdc-toolbar__section--align-end" role="toolbar">
<a href="#" class="material-icons mdc-toolbar__icon demo-toolbar__icon--custom" aria-label="Download" alt="Download">file_download</a>
<a href="#" class="material-icons mdc-toolbar__icon demo-toolbar__icon--custom" aria-label="Print this page" alt="Print this page">print</a>
<a href="#" class="material-icons mdc-toolbar__icon demo-toolbar__icon--custom" aria-label="Bookmark this page" alt="Bookmark this page">bookmark</a>
<span class="mdc-toolbar__icon demo-toolbar__icon--custom">
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably keep this an <a> element:

<a href="#" aria-label="Open in new window" alt="Open in new window">

Not only for consistency, but also because it makes the icon focusable and tabbable.

@@ -141,6 +141,14 @@ Icons can be added as anchor tags, `span`s, or `button`s to `mdc-toolbar`. There
`mdc-toolbar__menu-icon` represents the left most icon in `mdc-toolbar` usually to the left of `mdc-toolbar__title`.
`mdc-toolbar__icon` represents any icons placed on the right side of an `mdc-toolbar`.

When using `svg` icons, ensure you wrap the `svg` element in a `span` and include the `mdc-toolbar__icon` class.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should recommend either <a href="..."> or <span tabindex="0"> so that the icon is focusable and tabbable.

We should also make sure to include any necessary accessibility attributes (e.g., title or aria-label) in our examples.

@williamernest williamernest force-pushed the fix/toolbar/fix-svg-icon-color branch from 055fddf to 74d9a52 Compare March 6, 2018 22:33
Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

LGTM!

@williamernest williamernest merged commit 35a5cfc into master Mar 7, 2018
@williamernest williamernest deleted the fix/toolbar/fix-svg-icon-color branch March 7, 2018 00:54
acdvorak added a commit that referenced this pull request Mar 8, 2018
commit f17e0d3
Author: Will Ernest <[email protected]>
Date:   Thu Mar 8 13:20:48 2018 -0500

    fix(text-field): Clicking label should focus textfield (#2353)

commit 77b15f4
Author: Bonnie Zhou <[email protected]>
Date:   Wed Mar 7 16:29:57 2018 -0800

    refactor(button): Remove compact variant (#2361)

    BREAKING CHANGE: The compact variant of MDC Button is removed.

commit 35a5cfc
Author: Will Ernest <[email protected]>
Date:   Tue Mar 6 19:54:23 2018 -0500

    fix(toolbar): Fix colors for svg icons. Update custom-toolbar demo (#2331)

    SVG icons in the toolbar will now use the same color as the font icons.

commit dc52201
Author: Andrew C. Dvorak <[email protected]>
Date:   Tue Mar 6 16:00:12 2018 -0800

    fix(theme): Move @alternate annotations for Closure Stylesheets (#2355)

    `@alternate` annotations need to come before the _second_ property. Otherwise, Closure Compiler strips the first property (it does not output it at all).

commit 3c04419
Author: aprigogin <[email protected]>
Date:   Tue Mar 6 14:29:12 2018 -0800

    fix(button): icon in rtl should have margin right flipped. (#2346)

commit b9000a4
Author: Cory Reed <[email protected]>
Date:   Tue Mar 6 07:37:40 2018 -0800

    fix(demos): Correct RTL/LTR toggling in demos in Safari (#2348)

commit ab85736
Author: Andrew C. Dvorak <[email protected]>
Date:   Mon Mar 5 19:00:11 2018 -0500

    fix: Use `var` instead of `const` in menu demo (#2345)

    Safari 9.x and IE 10 do not support `const`

commit dc3d69f
Author: aprigogin <[email protected]>
Date:   Mon Mar 5 15:22:31 2018 -0800

    fix(rtl): Adding noflip annotations to fix downstream rtl issues (#2344)

    * fix(rtl): Adding noflip annotations to fix downstream rtl issues

    * fix(rtl): remove changes to button, will be in separate PR

    * fix(rtl): remove changes to button, will be in separate PR - second attempt.

    * fix(rtl): removed extra whitespace

commit eb4138e
Author: Patrick RoDee <[email protected]>
Date:   Mon Mar 5 14:10:46 2018 -0800

    docs: Update CHANGELOG.md

commit f478610
Author: Patrick RoDee <[email protected]>
Date:   Mon Mar 5 14:10:30 2018 -0800

    chore: Publish

commit 78408bb
Author: Andrew C. Dvorak <[email protected]>
Date:   Mon Mar 5 16:13:02 2018 -0500

    fix: Use `var` instead of `const` in demos/ready.js (#2343)

    Safari 9.x and IE 10 do not support `const`, and `ready.js` is not transpiled to ES5.

commit 49a9449
Author: Will Ernest <[email protected]>
Date:   Mon Mar 5 12:21:54 2018 -0500

    chore(select): Fix JS example in Readme (#2332)

commit bc17291
Author: Will Ernest <[email protected]>
Date:   Fri Mar 2 13:21:07 2018 -0500

    feat(top app bar): Add short top app bar always collapsed feature (#2327)

commit 0ba7d10
Author: Matty Goo <[email protected]>
Date:   Fri Mar 2 11:33:19 2018 -0500

    fix(text-field): disable validation check in setRequired (#2201)

    BREAKING CHANGE: removed setRequired and isRequired from foundation.

commit c14d244
Author: Will Ernest <[email protected]>
Date:   Fri Mar 2 10:37:18 2018 -0500

    chore(select): Fix ID values in demo (#2330)

    Remove unused ID and changed duplicated ID values. Also fixed the parentheses in RTL.

commit ecf4060
Author: Bonnie Zhou <[email protected]>
Date:   Thu Mar 1 16:38:41 2018 -0500

    feat(chips): Change chip color when selected (#2329)

    BREAKING CHANGE: The `mdc-chip--activated` class, `mdc-chip-activated-ink-color` Sass mixin, and the `toggleActive` methods on `MDCChip`/`MDCChipSet` have been renamed to `mdc-chip--selected`, `mdc-chip-selected-ink-color`, and `toggleSelected`, respectively.

commit 4b24b51
Author: Matty Goo <[email protected]>
Date:   Wed Feb 28 15:20:19 2018 -0500

    chore(floating-label): separate label module from text-field (#2237)

    BREAKING CHANGE: must use `.mdc-floating-label` selector instead of `.mdc-text-field__label`

commit fd8d8d9
Author: Will Ernest <[email protected]>
Date:   Wed Feb 28 14:21:55 2018 -0500

    feat(top-app-bar): Implement short top app bar (#2290)

    Adds the short top app bar variant to the top app bar.

commit a9f9bf2
Author: Kenneth G. Franqueiro <[email protected]>
Date:   Wed Feb 28 13:06:41 2018 -0500

    docs(select): Fix front matter for label sub-component (#2323)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants