Skip to content

[Fix] DateInput: isTouchDevice should only be determined in componentDidMount#336

Merged
ljharb merged 1 commit intomasterfrom
fix_dateinput_server_render
Feb 23, 2017
Merged

[Fix] DateInput: isTouchDevice should only be determined in componentDidMount#336
ljharb merged 1 commit intomasterfrom
fix_dateinput_server_render

Conversation

@ljharb
Copy link
Copy Markdown
Member

@ljharb ljharb commented Feb 20, 2017 •
edited
Loading

Fixes #335.

@ljharb ljharb added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Feb 20, 2017
@ljharb ljharb force-pushed the fix_dateinput_server_render branch from aebef0f to 18a439e Compare February 20, 2017 08:20
@coveralls
Copy link
Copy Markdown

coveralls commented Feb 20, 2017

Coverage Status

Coverage remained the same at 86.839% when pulling 18a439e on fix_dateinput_server_render into d58f6a9 on master.

@ljharb ljharb requested a review from majapw February 22, 2017 04:36
majapw
majapw requested changes Feb 23, 2017
View reviewed changes
Copy link
Copy Markdown
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

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

This is sweet. This is definitely the fix for #335. I wonder if this is also related to the problem with the Done button showing up for some people on iOS. I will clarify.

Comment thread src/components/DateInput.jsx
const { dateString } = this.state;
const {
dateString,
isTouchDevice: isTouch,
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.

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

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.

That identifier name is already used on line 5

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.

I see

Comment thread .eslintrc
"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

@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.

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 •
edited
Loading

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 •
edited
Loading

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

@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.

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

@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.

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

@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.

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

majapw
majapw approved these changes Feb 23, 2017
View reviewed changes
Copy link
Copy Markdown
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

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

Okeedoke.

Comment thread src/components/DateInput.jsx
const { dateString } = this.state;
const {
dateString,
isTouchDevice: isTouch,
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.

I see

@ljharb ljharb force-pushed the fix_dateinput_server_render branch from 18a439e to d978e54 Compare February 23, 2017 01:42
@coveralls
Copy link
Copy Markdown

coveralls commented Feb 23, 2017

Coverage Status

Coverage remained the same at 86.839% when pulling d978e54 on fix_dateinput_server_render into 333bbe5 on master.

@ljharb ljharb merged commit 04d08d4 into master Feb 23, 2017
@ljharb ljharb deleted the fix_dateinput_server_render branch February 23, 2017 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@majapw majapw majapw approved these changes

Assignees

No one assigned

Labels

semver-patch: fixes/refactors/etc Anything that's not major or minor.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ljharb @coveralls @majapw