fix(text field): adds missing adapter method `notifyIconAction` on Icon #600

mgr34 · 2019-01-08T15:49:55Z

Implements missing icon adapter method notifyIconAction on <Icon/>. Accomplished via adding optional onSelect to <Icon/> and corresponding onLeadingIconSelect and onTrailingIconSelect on <TextField/>. Still having trouble with screenshot tests. Should close #585.

Note: this is the first I've worked with typescript.

I signed it

…railingIconSelect).

codecov-io · 2019-01-08T15:55:25Z

Codecov Report

Merging #600 into rc0.9.0 will decrease coverage by 0.08%.
The diff coverage is 95%.

@@             Coverage Diff             @@
##           rc0.9.0     #600      +/-   ##
===========================================
- Coverage    94.45%   94.36%   -0.09%     
===========================================
  Files           61       61              
  Lines         2561     2575      +14     
  Branches       372      376       +4     
===========================================
+ Hits          2419     2430      +11     
- Misses          50       51       +1     
- Partials        92       94       +2

Impacted Files	Coverage Δ
packages/text-field/index.tsx	`97.56% <100%> (+0.06%)`	⬆️
packages/text-field/icon/index.tsx	`92% <91.66%> (-5.5%)`	⬇️

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 72daa92...9f30009. Read the comment docs.

moog16

Didn't a chance to look at the unit tests but thanks for adding them. I'll look more into them when i get back to a desktop.

moog16 · 2019-01-08T18:14:37Z

packages/text-field/icon/index.tsx

@@ -94,15 +108,27 @@ export default class Icon extends React.Component<
      removeAttr: (attr: keyof IconState) => (
        this.setState((prevState) => ({...prevState, [attr]: null}))
      ),
+      notifyIconAction: () => ( this.props.hasOwnProperty('onSelect')
+        ? this.props.onSelect!()


First time I'm doing a review on my phone... Do you have a default prop set for onSelect? We can get rid of the hasOwnProperty if you set one to an empty function

I think this might be an appropriate use for hasOwnProperty. Shortly I will be pushing a change where I eliminate it from the get tabidex().

I consciously chose against the approach of making it a defaultProp. Coincidentally, that is how I first went about it [0]. However, I ran into a problem where onSelect will only fire if tabindex != -1 due to pointer-events being set to none [1]. Since tabindex was not a required property for the passed icon I thought it more straightforward to add it based on if onSelect is present. If its has a default prop it will always be present and tabindex will always be initialized with 0 unless specified. This was the major factor in my decision to drop it as a default property.

I also seem to recall an issue in renderIcon from text-field where It was always passing an onSelect. Alas, I cannot quite recall the issue there and it was never committed. Maybe I was also setting a default prop for onLeadingIconSelect/onTrailingIconSelect?

Take your time on responding. A code review on a phone sounds painful.

[0] - df8627c#diff-78f850e429f25a98280e040459f8b54f

[1] https://github.com/material-components/material-components-web/blob/3cc4c6aa48ae8c1a307df542911e28a7808de41f/packages/mdc-textfield/icon/mdc-text-field-icon.scss#L33-L37

That makes sense about the defaultProp...but we don't use hasOwnProperty anywhere else in the library, which makes me suspicious. Either we should be doing that everywhere, or we shouldn't do it here - it just breaks the pattern. If we decide to do it, we should instead open another bug to do it system wide.

I hear you on that mindset....I guess I could do something like the following.

{ notifyIconAction: (() => this.props.onSelect ? this.props.onSelect() : null) }

for some reason I seem to rely heavily on hasOwnProperty() in other projects. Old habits and all... Nevertheless, making this exact change doesn't seem to cause any breaking changes. Will this work for you?

looks good to me!

packages/text-field/icon/index.tsx

test/screenshot/text-field/icon/index.tsx

moog16

please see new comments. Once they are fixed I'll review unit tests

packages/text-field/icon/index.tsx

packages/text-field/index.tsx

test/screenshot/text-field/icon/index.tsx

moog16 · 2019-01-08T19:01:01Z

test/screenshot/text-field/TextFieldPermutations.tsx

@@ -13,7 +13,11 @@ import {
 import TestField from './TestTextField';

 const textFields = (variantProps: {variant?: string}) => {
-  return iconsMap.map((icon: {leadingIcon?: React.ReactNode, trailingIcon?: React.ReactNode}) => {
+  return iconsMap.map((icon: {
+    leadingIcon?: React.ReactNode,


something i notice about these tests is that the icon tap target is only 24px x 24px. HIG (human interface guide - I think) says 48x48 is the minimum. I'm not sure what MDC Web does or if this is an issue with MDC Web. I'll follow up with one of our engineers.

moog16 · 2019-01-08T19:10:13Z

test/screenshot/text-field/TextFieldPermutations.tsx

@@ -13,7 +13,11 @@ import {
 import TestField from './TestTextField';

 const textFields = (variantProps: {variant?: string}) => {
-  return iconsMap.map((icon: {leadingIcon?: React.ReactNode, trailingIcon?: React.ReactNode}) => {
+  return iconsMap.map((icon: {


also getting an error when click on the leading-trailing-icon permutation. Try clicking on one of the icons here:

They also should not have tabIndex at all. It should be undefined or -1.

These all have a tabIidex of -1 for me, and thus have no pointer events. Nor should any instance of text-field that has both trailing and leading icons. At least that is how I am reading iconsMaps and TextFieldPermutations. Additionally, at this point get tabindex() in text-field will always set a tabindex. Not allowing it to be undefined.

cool - i think one of your other commits must have fixed it

moog16 · 2019-01-08T21:34:28Z

packages/text-field/icon/README.md

@@ -22,6 +22,9 @@ Prop Name | Type | Description
 --- | --- | ---
 disabled | Boolean | Toggles the disabled state of the icon.
 children | Element | Required. Expects a single child icon element.
+onSelect | Function() => void | Optional callback for user interaction with icon
+> Notes: `onSelect`  fired only for clicked event or when keydown event is enter key.


wording: `fired on click event and "enter key" keydown event.

moog16

looks good - minor spelling mistakes.

I will update with screenshot test changes. Please commit once i have them

test/unit/text-field/index.test.tsx

moog16 · 2019-01-08T22:06:18Z

@mgr34 please update text-field/icon golden.json to 0bbc8c762d27071e55983e5742548d166864b6fcebc0b9f1e413523fb93b7075

moog16 · 2019-01-08T22:43:34Z

tests are passing and pr is signed in #600 (comment). Merging!

…on (#600)

mgr34 added 4 commits January 3, 2019 16:50

fix(text-field): missing notifyIconAction adapter method added

df8627c

docs: add information about onSelect

43f1d5b

fix(text-field): missing notifyIconAction adapter method added

568fa4e

docs: add information about <Icon/> onSelect (onLeadingIconSelect,onT…

e22bb74

…railingIconSelect).

moog16 suggested changes Jan 8, 2019

View reviewed changes

mgr34 and others added 4 commits January 8, 2019 14:12

fix(text-field): remove gratuitous use of hasOwnProperty

1573736

fix(text-field): removal of hasOwnProperty

72c4ef8

fix(text-field): disabled is an optional property

e0553e4

Merge branch 'rc0.9.0' into fix/text-field/icon

2098e08

moog16 reviewed Jan 8, 2019

View reviewed changes

moog16 suggested changes Jan 8, 2019

View reviewed changes

test/unit/text-field/index.test.tsx Outdated Show resolved Hide resolved

test/unit/text-field/index.test.tsx Outdated Show resolved Hide resolved

Matt Goo and others added 2 commits January 8, 2019 13:45

Merge branch 'rc0.9.0' into fix/text-field/icon

51492a0

fix(text-field): spelling errors

1eadc24

fix(text-field): update golden.json for text-field/icon

9f30009

moog16 mentioned this pull request Jan 8, 2019

DO NOT MERGE: Tests Mgr34 fix/text field/icon #602

Closed

moog16 merged commit f92cc2e into material-components:rc0.9.0 Jan 8, 2019

moog16 added this to the 0.9.0 milestone Jan 8, 2019

moog16 pushed a commit that referenced this pull request Jan 23, 2019

fix(text field): adds missing adapter method notifyIconAction on Ic…

632896a

…on (#600)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(text field): adds missing adapter method `notifyIconAction` on Icon #600

fix(text field): adds missing adapter method `notifyIconAction` on Icon #600

mgr34 commented Jan 8, 2019

codecov-io commented Jan 8, 2019 •

edited

moog16 left a comment

moog16 Jan 8, 2019

mgr34 Jan 8, 2019

moog16 Jan 8, 2019

mgr34 Jan 8, 2019

moog16 Jan 8, 2019

moog16 left a comment

moog16 Jan 8, 2019

moog16 Jan 8, 2019 •

edited

mgr34 Jan 8, 2019

moog16 Jan 8, 2019

moog16 Jan 8, 2019

moog16 left a comment

moog16 commented Jan 8, 2019

moog16 commented Jan 8, 2019

fix(text field): adds missing adapter method notifyIconAction on Icon #600

fix(text field): adds missing adapter method notifyIconAction on Icon #600

Conversation

mgr34 commented Jan 8, 2019

codecov-io commented Jan 8, 2019 • edited

Codecov Report

moog16 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

Choose a reason for hiding this comment

moog16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moog16 Jan 8, 2019 • edited

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

moog16 left a comment

Choose a reason for hiding this comment

moog16 commented Jan 8, 2019

moog16 commented Jan 8, 2019

fix(text field): adds missing adapter method `notifyIconAction` on Icon #600

fix(text field): adds missing adapter method `notifyIconAction` on Icon #600

codecov-io commented Jan 8, 2019 •

edited

moog16 Jan 8, 2019 •

edited