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

Improve touch tap performance #412

Merged
merged 3 commits into from
Mar 31, 2017
Merged

Conversation

moonboots
Copy link
Collaborator

@moonboots moonboots commented Mar 30, 2017

to: @majapw @lencioni

This PR skips expensive modifier calculations that slow down render performance. These modifiers are computed for every calendar day, which becomes problematic on mobile if we're showing more months.

I instrumented the time spent in each modifier and tested on my Pixel. The total time is ~400ms. 70ms were spent for the today check, and 60ms were spent in the hover checks. I've disabled hover for mobile and added the optional to skip the today check.

The calculateToday prop isn't the cleanest approach, but I think the perf problem is serious enough to address immediately as we work on a more general fix. On this note, I've discussed an approach with Maja of computing these modifiers strictly at the top level instead of lazily at the leaves. This will allow optimizations like only computing today once and only recomputing affected date ranges.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.322% when pulling 76c19cf on moonboots:jack-tap-perf into f1cee85 on airbnb:master.

@@ -89,6 +90,7 @@ const defaultProps = {

renderDay: null,
renderCalendarInfo: null,
calculateToday: true,
Copy link
Member

Choose a reason for hiding this comment

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

ideally boolean props always default to false, such that absence and false are the same - the main exception is when the default is intended to be flipped later in a semver-major bump. If that's not the case here, could this be renamed so that it can default to false?

@moonboots
Copy link
Collaborator Author

moonboots commented Mar 30, 2017

@majapw I removed the calculateToday commit (will keep this handy 😉 ) ptal

Copy link
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.

Let's maybe conditionally send down some of the other modifiers as well, but let's separate out the calculateToday change into a separate PR like we discussed today.

// while start date has been set, but end date has not been
'hovered-span': day => this.isInHoveredSpan(day),
'after-hovered-start': day => this.isDayAfterHoveredStartDate(day),
'last-in-range': day => this.isLastInRange(day),

// once a start date and end date have been set
'selected-start': day => this.isStartDate(day),
Copy link
Collaborator

@majapw majapw Mar 30, 2017

Choose a reason for hiding this comment

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

What if we did something similar for the selection related ones?

selected-start requires START_DATE to exist,

selected-end and blocked-minimum-nights require END_DATE,

selected-span and last-in-range require both,

we could apply those conditionals within the touch params as well.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.322% when pulling 1988d19 on moonboots:jack-tap-perf into f1cee85 on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.322% when pulling fd4890c on moonboots:jack-tap-perf into f1cee85 on airbnb:master.

Copy link
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.

Looks great to me.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.322% when pulling d2e2320 on moonboots:jack-tap-perf into f1cee85 on airbnb:master.

@majapw majapw merged commit fafa451 into react-dates:master Mar 31, 2017
@palashkaria
Copy link

palashkaria commented May 17, 2017

Hey, I came across this part of the code, white trying to make a DayPickerSingleDateController and I'm not sure how ...startDate && { .. } is working.
Can someone explain what they are doing here? startDate is a moment obj, so it throws an error when I try this in console. @moonboots @majapw

@majapw
Copy link
Collaborator

majapw commented May 17, 2017

Hi @palashkaria, this code doesn't even exist anymore in react-dates but what it was doing was spreading the result of startDate && { ... } onto the object. if startDate is a moment object and therefore truthy, then startDate && { ... } just evaluates to { ... } which is spread onto the parent object. If startDate is falsey, ie null or something similar, then startDate && { ... } resolves to null which does nothing when passed into the spread operator.

Spreading (leading ...) means putting then contents of the object onto the parent. Basically, it's the new Object.assign.

@palashkaria
Copy link

Hey, @majapw
I tried this in chrome, and it didn't work, probably not supported by chrome yet in this syntax, hence I got confused

I see that now you do modifiers in CWRP, right? I was probably looking at an old version. Thanks for the quick explaination.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants