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(text field): adds missing adapter method notifyIconAction on Icon #600

Merged
merged 11 commits into from
Jan 8, 2019
12 changes: 8 additions & 4 deletions packages/text-field/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ React Text Field accepts one child element which is the input element. For ease
```js
import React from 'react';
import TextField, {HelperText, Input} from '@material/react-text-field';
import MaterialIcon from from '@material/react-material-icon';

class MyApp extends React.Component {
state = {value: 'Woof'};
Expand All @@ -39,10 +40,11 @@ class MyApp extends React.Component {
<TextField
label='Dog'
helperText={<HelperText>Help Me!</HelperText>}
>
<Input
value={this.state.value}
onChange={(e) => this.setState({value: e.target.value})}/>
onTrailingIconSelect={() => this.setState({value: ''})}
trailingIcon={<MaterialIcon role="button" icon="delete"/>}
><Input
value={this.state.value}
onChange={(e) => this.setState({value: e.target.value})} />
</TextField>
</div>
);
Expand All @@ -66,6 +68,8 @@ label | String | Mandatory. Label text that appears as the floating label or pla
leadingIcon | Element | An icon element that appears as the leading icon.
lineRippleClassName | String | An optional class added to the line ripple element.
notchedOutlineClassName | String | An optional class added to the notched outline element.
onLeadingIconSelect | Function | Optional callback fired on interaction with `leadingIcon`.
onTrailingIconSelect | Function | Optional callback fired on interaction with `trailingIcon`.
outlined | Boolean | Enables outlined variant.
textarea | Boolean | Enables textarea variant.
trailingIcon | Element | An icon element that appears as the trailing icon.
Expand Down
3 changes: 3 additions & 0 deletions packages/text-field/icon/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 on click event and "Enter key" keydown event.
> `onSelect` will add tabindex of 0 if tabindex is not previously added to icon

## Icon

Expand Down
32 changes: 29 additions & 3 deletions packages/text-field/icon/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ import * as classnames from 'classnames';
import {MDCTextFieldIconFoundation} from '@material/textfield/dist/mdc.textfield';

export interface IconProps extends React.HTMLProps<HTMLOrSVGElement> {
disabled: boolean;
disabled?: boolean;
children: React.ReactElement<React.HTMLProps<HTMLOrSVGElement>>;
onSelect?: () => void;
mgr34 marked this conversation as resolved.
Show resolved Hide resolved
};

interface IconState {
Expand All @@ -47,12 +48,11 @@ export default class Icon extends React.Component<
constructor(props: IconProps) {
super(props);
const {
tabIndex: tabindex, // note that foundation.js alters tabindex not tabIndex
role,
} = props.children.props;

this.state = {
tabindex,
tabindex: this.tabindex,
role,
};
}
Expand All @@ -69,6 +69,10 @@ export default class Icon extends React.Component<
if (this.props.disabled !== prevProps.disabled) {
this.foundation_.setDisabled(this.props.disabled);
}

if (this.props.onSelect !== prevProps.onSelect) {
this.setState({tabindex: this.tabindex});
}
}

componentWillUnmount() {
Expand All @@ -77,6 +81,16 @@ export default class Icon extends React.Component<
}
}

get tabindex() {
// if tabIndex is not set onSelect will never fire.
// note that foundation.js alters tabindex not tabIndex
if (typeof this.props.children.props.tabIndex === 'number') {
return this.props.children.props.tabIndex;
}

return this.props.onSelect ? 0 : -1;
}

get adapter() {
return {
// need toString() call when tabindex === 0.
Expand All @@ -94,15 +108,27 @@ export default class Icon extends React.Component<
removeAttr: (attr: keyof IconState) => (
this.setState((prevState) => ({...prevState, [attr]: null}))
),
notifyIconAction: () => ( this.props.onSelect
? this.props.onSelect()
: null
),
};
}

handleClick = (e: React.MouseEvent<HTMLElement>) =>
this.foundation_.handleInteraction(e);

handleKeyDown = (e: React.KeyboardEvent<HTMLElement>) =>
this.foundation_.handleInteraction(e)

addIconAttrsToChildren = () => {
const {tabindex: tabIndex, role} = this.state;
const child = React.Children.only(this.props.children);
const className = classnames('mdc-text-field__icon', child.props.className);
const props = Object.assign({}, child.props, {
className,
onClick: this.handleClick,
onKeyDown: this.handleKeyDown,
tabIndex,
role,
});
Expand Down
15 changes: 11 additions & 4 deletions packages/text-field/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ export interface Props<T> {
lineRippleClassName: string;
notchedOutlineClassName: string;
outlined: boolean;
onLeadingIconSelect?: () => void;
mgr34 marked this conversation as resolved.
Show resolved Hide resolved
onTrailingIconSelect?: () => void;
textarea: boolean;
trailingIcon?: React.ReactElement<React.HTMLProps<HTMLOrSVGElement>>;
};
Expand Down Expand Up @@ -172,6 +174,8 @@ class TextField<T extends {}> extends React.Component<TextFieldProps<T>, TextFie
leadingIcon,
lineRippleClassName,
notchedOutlineClassName,
onLeadingIconSelect,
onTrailingIconSelect,
outlined,
textarea,
trailingIcon,
Expand Down Expand Up @@ -309,6 +313,8 @@ class TextField<T extends {}> extends React.Component<TextFieldProps<T>, TextFie
fullWidth,
helperText,
outlined,
onLeadingIconSelect,
onTrailingIconSelect,
leadingIcon,
trailingIcon,
textarea,
Expand All @@ -322,12 +328,12 @@ class TextField<T extends {}> extends React.Component<TextFieldProps<T>, TextFie
onKeyDown={() => foundation && foundation.handleTextFieldInteraction()}
key='text-field-container'
>
{leadingIcon ? this.renderIcon(leadingIcon) : null}
{leadingIcon ? this.renderIcon(leadingIcon, onLeadingIconSelect) : null}
{foundation ? this.renderInput() : null}
{label && !fullWidth ? this.renderLabel() : null}
{outlined ? this.renderNotchedOutline() : null}
{!fullWidth && !textarea && !outlined ? this.renderLineRipple() : null}
{trailingIcon ? this.renderIcon(trailingIcon) : null}
{trailingIcon ? this.renderIcon(trailingIcon, onTrailingIconSelect) : null}
</div>
);

Expand Down Expand Up @@ -409,10 +415,11 @@ class TextField<T extends {}> extends React.Component<TextFieldProps<T>, TextFie
return React.cloneElement(helperText, props);
}

renderIcon(icon: React.ReactElement<React.HTMLProps<HTMLOrSVGElement>>) {
renderIcon(icon: React.ReactElement<React.HTMLProps<HTMLOrSVGElement>>,
onSelect?: () => void) {
const {disabled} = this.state;
// Toggling disabled will trigger icon.foundation.setDisabled()
return <Icon disabled={disabled}>{icon}</Icon>;
return <Icon disabled={disabled} onSelect={onSelect}>{icon}</Icon>;
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/screenshot/golden.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"tab-indicator": "7ce7ce8fd50301c67d7ebfb0ba953208260ce2881bee0c7e640c46bf60dc90b6",
"tab-scroller": "468866dd0c222b36b55485ab44a5760133a4ddfb2a6cf81e6ae4672d7e02a447",
"text-field/helper-text": "59604d0f39e0846fc97aae7573d317dded215282a677e4641c5e33426e3a2a1e",
"text-field/icon": "c34dae5444deec222533b3f43448c0393b0c8543a5af4e50cc12d71611f366a7",
"text-field/icon": "0bbc8c762d27071e55983e5742548d166864b6fcebc0b9f1e413523fb93b7075",
"text-field/textArea": "871b32d2fd1982e191e9d5f6ac32e8eb4d82f9fbb1a83359bce8e8f2e9edd027",
"text-field/standard": "be2ea75813583dac8d3ad988282cfa19fa7266a29b095cbd60a34912a1900251",
"text-field/fullWidth": "7c854723b1b4ce7e6df614f44f7b7845bef6098ac30714da5c5128bbd57eb51f",
Expand All @@ -44,4 +44,4 @@
"drawer/modal": "da83487c9349b253f7d4de01f92d442de55aab92a8028b77ff1a48cfaa265b72",
"drawer/permanentToModal": "6355905c2241b5e6fdddc2e25119a1cc3b062577375a88b59e6750c4b76e4561",
"typography": "c5e87d672d8c05ca3b61c0df4971eabe3c6a6a1f24a9b98f71f55a23360c498a"
}
}
6 changes: 5 additions & 1 deletion test/screenshot/text-field/TextFieldPermutations.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Copy link
Contributor

@moog16 moog16 Jan 8, 2019

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

leadingIcon?: React.ReactNode,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

onLeadingIconSelect?: () => void,
trailingIcon?: React.ReactNode,
onTrailingIconSelect?: () => void, }) => {
return denseMap.map((dense: {dense?: boolean}) => {
return rtlMap.map((isRtl: {isRtl?: boolean}) => {
return requiredMap.map((isRequired: {required?: boolean}) => {
Expand Down
4 changes: 2 additions & 2 deletions test/screenshot/text-field/attributesMap.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ const iconAlt = <MaterialIcon icon='work' />;

const iconsMap = [
{},
{leadingIcon: icon},
{trailingIcon: icon},
{leadingIcon: icon, onLeadingIconSelect: () => console.log('bark')},
{trailingIcon: icon, onTrailingIconSelect: () => console.log('shhh')},
{leadingIcon: icon, trailingIcon: iconAlt},
];
const denseMap = [{}, {dense: true}];
Expand Down
4 changes: 4 additions & 0 deletions test/screenshot/text-field/icon/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ const TextFieldIconScreenshotTest = () => {
<Icon>
<MaterialIcon tabIndex={0} role='button' icon='favorite' />
</Icon>

mgr34 marked this conversation as resolved.
Show resolved Hide resolved
<Icon onSelect={() => console.log('❤️')}>
<MaterialIcon role='button' icon='favorite' />
</Icon>
</div>
);
};
Expand Down
64 changes: 63 additions & 1 deletion test/unit/text-field/icon/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {assert} from 'chai';
import {shallow} from 'enzyme';
import Icon from '../../../../packages/text-field/icon/index';
import MaterialIcon from '../../../../packages/material-icon/index';
import {coerceForTesting} from '../../helpers/types';

suite('Text Field Icon');

Expand Down Expand Up @@ -59,7 +60,7 @@ test('#componentDidMount calls #foundation.setDisabled if disabled prop is true'
<i />
</Icon>
);
assert.equal(wrapper.state().tabindex, undefined);
assert.equal(wrapper.state().tabindex, -1); // foundation setDisabled sets tabIndex -1
assert.equal(wrapper.state().role, undefined);
});

Expand All @@ -73,6 +74,26 @@ test('#componentDidMount calls #foundation.setDisabled if disabled prop is true
assert.equal(wrapper.state().role, undefined);
});

test('#componentDidMount sets tabindex if prop not present but onSelect exists', () => {
// w/out tabindex onSelect will never fire && cursor: pointer is not applied
const wrapper = shallow<Icon>(
<Icon onSelect={() => {}}><MaterialIcon icon='favorite' /></Icon>);

assert.equal(wrapper.state().tabindex, 0);
});

test(
'#componentDidUpdate will set tabindex if prop not present but updates ' +
'with onSelect',
() => {
const wrapper = shallow<Icon>(<Icon><MaterialIcon icon='favorite' /></Icon>);

assert.equal(wrapper.state().tabindex, -1);
wrapper.setProps({onSelect: () => {}});
assert.equal(wrapper.state().tabindex, 0);
}
);

test(
'#componentDidUpdate calls #foundation.setDisabled if ' +
'props.disabled updates',
Expand Down Expand Up @@ -201,6 +222,47 @@ test('#adapter.removeAttr for role works with Custom Component', () => {
assert.equal(wrapper.state().role, undefined);
});

test('#adapter.notifyIconAction calls props.onSelect', () => {
const onSelect = coerceForTesting<() => void>(td.func());
const wrapper = shallow<Icon>(
<Icon onSelect={onSelect}>
<MaterialIcon icon='favorite' role='button' />
</Icon>
);
wrapper.instance().foundation_.adapter_.notifyIconAction();
td.verify(onSelect(), {times: 1});
});

test('onClick calls foundation.handleInteraction', () => {
const onSelect = coerceForTesting<() => void>(td.func());
const wrapper = shallow<Icon>(
<Icon onSelect={onSelect}>
<MaterialIcon icon='favorite' role='button' />
</Icon>
);
const evt = coerceForTesting<React.MouseEvent>({});
wrapper.instance().foundation_.handleInteraction = td.func();
wrapper.simulate('click', evt);
td.verify(wrapper.instance().foundation_.handleInteraction(evt), {
times: 1,
});
});

test('onKeyDown call foundation.handleInteraction', () => {
const onSelect = coerceForTesting<() => void>(td.func());
const wrapper = shallow<Icon>(
<Icon onSelect={onSelect}>
<MaterialIcon icon='favorite' role='button' />
</Icon>
);
const evt = coerceForTesting<React.KeyboardEvent>({});
wrapper.instance().foundation_.handleInteraction = td.func();
wrapper.simulate('keydown', evt);
td.verify(wrapper.instance().foundation_.handleInteraction(evt), {
times: 1,
});
});

test('updating the role reflects on DOM node', () => {
const wrapper = shallow(
<Icon>
Expand Down