feat(toolbar): add pointer for icon element

[Shyam-Chen]

No description provided.

[codecov-io]

Codecov Report

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

@@           Coverage Diff           @@
##           master     #974   +/-   ##
=======================================
  Coverage   99.93%   99.93%           
=======================================
  Files          68       68           
  Lines        3276     3276           
  Branches      403      403           
=======================================
  Hits         3274     3274           
  Misses          2        2

---

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 ffb9023...ee2cfa8. Read the comment docs.

[yeelan0319]

&__icon is separate from &__icon--menu, so we need to add this rule to both.

[Shyam-Chen]

done.

[Garbee]

This seems icky to me. The icons shouldn't trigger anything. In fact they should be pointer-events: none so they don't trigger any events in JS or anything in-accurately. It is the underlying buttons that should have the hover triggers and event listeners if any are to exist.

[yeelan0319]

@Garbee Curious about your suggestion here "icon shouldn't trigger anything. It is the underlying buttons that should have the hover triggers". Does this means a button need to be nested within &__icon if the icon is intended to have an action? Such as:

<header class="mdc-toolbar mdc-toolbar--fixed">
  <div class="mdc-toolbar__row">
    <div class="mdc-toolbar__icon" aria-label="Download" alt="Download">
      <button><i class="material-icons">file_download</i></button>
    </div>
  </div>
</header>

IMHO, this makes toolbar structure bulkier than it is but the gain here is pretty limited, since based on the guideline that all the icons within toolbar should be actionable. Correct me if I get your meaning wrong.

[Garbee]

Ah, I see now looking at the code more. The class naming changed some from what we were initially demoing for the component early on. This LGTM knowing that.

[yeelan0319]

Yeah, there were some other issues related to those icons so we renamed it.
I saw you filed a couple of issues regarding the naming, let's triage them later on.