Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@
"rules": {
"no-mixed-operators": [2, { "allowSamePrecedence": true }],
"jsx-a11y/no-static-element-interactions": 1, // TODO: enable
"react/no-did-mount-set-state": 0, // necessary for server-rendering
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we exclusively put this on the DateInput instead of as a universal exception?

Copy link
Copy Markdown
Member Author

@ljharb ljharb Feb 23, 2017

Choose a reason for hiding this comment

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

I can, but it should be a universal exception, because it's necessary for any component to be server-rendered. I need to disable this rule upstream in the airbnb config too - setState in componentDidMount is a good thing.

Copy link
Copy Markdown
Collaborator

@majapw majapw Feb 23, 2017

Choose a reason for hiding this comment

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

Really? we're fine with a double-render on first mount? I feel like that's something we should work against.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Every single one of airbnb's hypernova components works like that already. We can definitely strive to avoid it, but it's not possible to avoid for client-required functionality.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess that's fair, but given that this repo is small, I would prefer to turn it off on a case by case basis.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If you prefer that I can do it - but we might need to make the same change in any component that has touch detection.

}
}
12 changes: 9 additions & 3 deletions src/components/DateInput.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,15 @@ export default class DateInput extends React.Component {
super(props);
this.state = {
dateString: '',
isTouchDevice: false,
};

this.onChange = this.onChange.bind(this);
this.onKeyDown = this.onKeyDown.bind(this);
}

this.isTouchDevice = isTouchDevice();
componentDidMount() {
this.setState({ isTouchDevice: isTouchDevice() });
}

componentWillReceiveProps(nextProps) {
Expand Down Expand Up @@ -89,7 +92,10 @@ export default class DateInput extends React.Component {
}

render() {
const { dateString } = this.state;
const {
dateString,
isTouchDevice: isTouch,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm confused about the need for this name change. Can't we just keep this as isTouchDevice?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That identifier name is already used on line 5

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see

} = this.state;
const {
id,
placeholder,
Expand Down Expand Up @@ -128,7 +134,7 @@ export default class DateInput extends React.Component {
placeholder={placeholder}
autoComplete="off"
disabled={disabled}
readOnly={this.isTouchDevice}
readOnly={isTouch}
required={required}
aria-describedby={screenReaderMessage && screenReaderMessageId}
/>
Expand Down
13 changes: 13 additions & 0 deletions test/_helpers/withTouchSupport.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import wrap from 'mocha-wrap';
import withGlobal from 'mocha-wrap/withGlobal';
import withOverride from 'mocha-wrap/withOverride';

function withTouchSupport() {
return this
.use(withGlobal, 'window', () => (typeof window !== 'undefined' ? window : {}))
.use(withOverride, () => window, 'ontouchstart', () => window.ontouchstart || (() => {}))
.use(withGlobal, 'navigator', () => (typeof navigator !== 'undefined' ? navigator : {}))
.use(withOverride, () => navigator, 'maxTouchPoints', () => navigator.maxTouchPoints || true);
}

wrap.register(withTouchSupport);
28 changes: 28 additions & 0 deletions test/components/DateInput_spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import { expect } from 'chai';
import { shallow } from 'enzyme';
import sinon from 'sinon-sandbox';
import wrap from 'mocha-wrap';

import DateInput from '../../src/components/DateInput';

Expand Down Expand Up @@ -206,4 +207,31 @@ describe('DateInput', () => {
expect(onKeyDownTabStub.callCount).to.equal(0);
});
});

describe('touch device detection', () => {
it('indicates no touch support on the client', () => {
const wrapper = shallow(<DateInput id="date" />);
expect(wrapper.state()).to.contain.keys({ isTouchDevice: false });
});

it('does not set readOnly when not a touch device', () => {
const wrapper = shallow(<DateInput id="date" />);
expect(!!wrapper.find('input').prop('readOnly')).to.equal(false);
});

it('sets readOnly when a touch device', () => {
const wrapper = shallow(<DateInput id="date" />);
wrapper.setState({ isTouchDevice: true });
wrapper.update();
expect(!!wrapper.find('input').prop('readOnly')).to.equal(true);
});

wrap()
.withTouchSupport()
.it('sets isTouchDevice state when is a touch device', () => {
const wrapper = shallow(<DateInput id="date" />);
wrapper.instance().componentDidMount();
expect(wrapper.state()).to.contain.keys({ isTouchDevice: true });
});
});
});