-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[Fix] DateInput: isTouchDevice should only be determined in componentDidMount
#336
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
|
@@ -89,7 +92,10 @@ export default class DateInput extends React.Component { | |
| } | ||
|
|
||
| render() { | ||
| const { dateString } = this.state; | ||
| const { | ||
| dateString, | ||
| isTouchDevice: isTouch, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That identifier name is already used on line 5
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see |
||
| } = this.state; | ||
| const { | ||
| id, | ||
| placeholder, | ||
|
|
@@ -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} | ||
| /> | ||
|
|
||
| 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); |
There was a problem hiding this comment.
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
DateInputinstead of as a universal exception?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 -
setStateincomponentDidMountis a good thing.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.