Skip to content
This repository was archived by the owner on Jan 14, 2025. It is now read-only.

fix(icon-button): upgrade to mdc-web v1.1 #792

Merged
merged 1 commit into from Apr 2, 2019
Merged

fix(icon-button): upgrade to mdc-web v1.1 #792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 43 additions & 17 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
"@material/drawer": "^1.0.1",
"@material/fab": "^1.1.0",
"@material/floating-label": "^1.0.0",
"@material/icon-button": "^0.41.0",
"@material/icon-button": "^1.1.0",
"@material/layout-grid": "^0.41.0",
"@material/line-ripple": "^1.0.0",
"@material/linear-progress": "^1.1.0",
Expand Down
15 changes: 10 additions & 5 deletions packages/icon-button/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@
import * as React from 'react';
import classnames from 'classnames';
import * as Ripple from '@material/react-ripple';
// @ts-ignore no mdc .d.ts file
import {MDCIconButtonToggleFoundation} from '@material/icon-button/dist/mdc.iconButton';
import {MDCIconButtonToggleAdapter} from '@material/icon-button/adapter';
import {MDCIconButtonToggleFoundation} from '@material/icon-button/foundation';
import {MDCIconButtonToggleEventDetail} from '@material/icon-button/types';
import IconToggle from './IconToggle';
const ARIA_PRESSED = 'aria-pressed';

Expand All @@ -35,6 +36,7 @@ interface ElementAttributes {

type IconButtonTypes = HTMLButtonElement | HTMLAnchorElement;
export interface IconButtonBaseProps extends ElementAttributes {
handleChange?: (evt: MDCIconButtonToggleEventDetail) => void;
isLink?: boolean;
};

Expand All @@ -49,7 +51,7 @@ class IconButtonBase<T extends IconButtonTypes> extends React.Component<
IconButtonProps<T>,
IconButtonBaseState
> {
foundation = MDCIconButtonToggleFoundation;
foundation!: MDCIconButtonToggleFoundation;

constructor(props: IconButtonProps<T>) {
super(props);
Expand All @@ -61,6 +63,7 @@ class IconButtonBase<T extends IconButtonTypes> extends React.Component<

static defaultProps = {
className: '',
handleChange: () => {},
initRipple: () => {},
isLink: false,
onClick: () => {},
Expand All @@ -78,17 +81,18 @@ class IconButtonBase<T extends IconButtonTypes> extends React.Component<
return classnames('mdc-icon-button', Array.from(classList), className);
}

get adapter() {
get adapter(): MDCIconButtonToggleAdapter {
return {
addClass: (className: string) =>
this.setState({classList: this.state.classList.add(className)}),
removeClass: (className: string) => {
const classList = new Set(this.state.classList);
const {classList} = this.state;
Copy link
Author

Choose a reason for hiding this comment

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

This change is done because hasClass would otherwise return a false positive if it were called after this. This led to the isOn property in the event argument for notifyChange always being true.

This pattern seems to be used throughout this library and it is a little worrying as it goes against the first two points that react advises about for using state correctly: https://reactjs.org/docs/state-and-lifecycle.html#using-state-correctly

However, from what I can tell, it needs to work this way in order for hasClass to behave correctly.

Copy link

Choose a reason for hiding this comment

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

Gotcha - ya it I realize that setState is async, and that it can become out of sync with hasClass. It does seem wrong that we are relying on changing state directly without calling setState. That is why we were calling new Set() to create a new instance.

We could possibly make the call to props.handleChange call async, by calling it in the componentDidUpdate. We could use a boolean indicating state.classList is updating (let's call it state.classListIsUpdating. When notifyChange gets trigger, we could set another boolean state.hasChangeOccured. (hopefully you're following). The componentDidUpdate statement would look like:

if (!state.classListIsUpdating && prevState.classListIsUpdating && state.hasChangeOccured) {
  this.props.handleChange(...);
  this.setState({hasChangeOccured: false}); // clean up to setup for the next notifyChange 
}

Just a thought as a fix for this. It definitely doesn't belong in this PR, but maybe we can open another issue to fix this.

Copy link

Choose a reason for hiding this comment

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

And we should actually be consistent in the removeClass and addClass methods...
Created an issue #793

Copy link
Author

Choose a reason for hiding this comment

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

@moog16 thanks for the feedback and creating a new issue for this.

classList.delete(className);
this.setState({classList});
},
hasClass: (className: string) => this.classes.split(' ').includes(className),
setAttr: this.updateState,
notifyChange: this.props.handleChange!,
};
}

Expand All @@ -111,6 +115,7 @@ class IconButtonBase<T extends IconButtonTypes> extends React.Component<
isLink,
/* eslint-disable no-unused-vars */
className,
handleChange,
onClick,
unbounded,
[ARIA_PRESSED]: ariaPressed,
Expand Down
2 changes: 1 addition & 1 deletion packages/icon-button/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"url": "https://github.com/material-components/material-components-web-react.git"
},
"dependencies": {
"@material/icon-button": "^0.41.0",
"@material/icon-button": "^1.1.0",
"@material/react-ripple": "^0.11.0",
"classnames": "^2.2.6",
"react": "^16.3.2"
Expand Down
18 changes: 13 additions & 5 deletions test/unit/icon-button/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as td from 'testdouble';
import {shallow} from 'enzyme';
import {IconButtonBase as IconButton} from '../../../packages/icon-button/index';
import {coerceForTesting} from '../helpers/types';
import {MDCIconButtonToggleEventDetail} from '@material/icon-button/types';

suite('IconButton');

Expand Down Expand Up @@ -34,7 +35,7 @@ test('renders anchor tag when isLink is true', () => {

test('#foundation.handleClick gets called onClick', () => {
const wrapper = shallow<IconButton<HTMLButtonElement>>(<IconButton />);
wrapper.instance().foundation.handleClick = td.func();
wrapper.instance().foundation.handleClick = td.func<() => void>();
wrapper.simulate('click');
td.verify(wrapper.instance().foundation.handleClick(), {times: 1});
});
Expand Down Expand Up @@ -62,13 +63,13 @@ test('aria-pressed is set false if passed as prop but on className is not passed
test('#get.classes has a class added to state.classList', () => {
const wrapper = shallow(<IconButton />);
wrapper.setState({classList: new Set(['test-class'])});
wrapper.hasClass('test-class');
assert.isTrue(wrapper.hasClass('test-class'));
});

test('#adapter.addClass adds a class to state.classList', () => {
const wrapper = shallow<IconButton<HTMLButtonElement>>(<IconButton />);
wrapper.instance().adapter.addClass('test-class');
wrapper.state().classList.has('test-class');
assert.isTrue(wrapper.state().classList.has('test-class'));
Copy link

Choose a reason for hiding this comment

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

wow - these tests were basically useless...that's for fixing these.

});

test('#adapter.removeClass removes a class to state.classList', () => {
Expand All @@ -91,7 +92,14 @@ test('#adapter.hasClass returns false if element does not contains class', () =>

test('#adapter.setAttr sets aria-pressed', () => {
const wrapper = shallow<IconButton<HTMLButtonElement>>(<IconButton />);
wrapper.instance().adapter.setAttr('aria-pressed', true);
assert.isTrue(wrapper.state()['aria-pressed']);
wrapper.instance().adapter.setAttr('aria-pressed', 'true');
assert.equal(wrapper.state('aria-pressed'), 'true');
});

test('#adapter.notifyChange calls props.handleChange', () => {
const handleChange = td.func<(event: MDCIconButtonToggleEventDetail) => void>();
const wrapper = shallow<IconButton<HTMLButtonElement>>(<IconButton handleChange={handleChange} />);
const event = {isOn: false};
wrapper.instance().adapter.notifyChange(event);
td.verify(handleChange(event), {times: 1});
});