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

Avoid rendering DayPicker when not visible #286

Merged
merged 1 commit into from
Feb 7, 2017

Conversation

lencioni
Copy link
Contributor

@lencioni lencioni commented Feb 1, 2017

I was looking at some markup that we generate during server rendering
and noticed a whole bunch of markup (~32 KiB of HTML) belonging to
react-dates for elements that were not visible. With a couple of minor
modifications, we can avoid rendering most of these nodes until the
input is actually focused and we want to open the DayPicker.


To @airbnb/webinfra @majapw

If you don't see any problems with this approach, I'll go ahead and update the tests.

@majapw
Copy link
Collaborator

majapw commented Feb 1, 2017

I'll check out the branch and play around with it.

@majapw
Copy link
Collaborator

majapw commented Feb 2, 2017

I'm seeing a proptype warning:

Warning: Failed prop type: The prop `children` is marked as required in `Portal`, but its value is `null`.
    in Portal (created by SingleDatePicker)
    in SingleDatePicker (created by SingleDatePickerWrapper)
    in SingleDatePickerWrapper
    in div (created by Story)
    in div (created by Story)
    in Story

I think this is likely because due to your change, this.renderDayPicker() sometimes returns null. Maybe that logic should be moved into this.maybeRenderDayPickerWithPortal() so as not to render the portal either in the case that the picker is hidden.

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.

From playing around with the storybook, this looks and feels pretty good! We need to address the tests and also address the prop type warning as stated in my other comment, but over seems legit.

I was looking at some markup that we generate during server rendering
and noticed a whole bunch of markup (~32 KiB of HTML) belonging to
react-dates for elements that were not visible. With a couple of minor
modifications, we can avoid rendering most of these nodes until the
input is actually focused and we want to open the DayPicker.

This should give us a nice performance boost for both server-rendered
pages with this component and again on the client when rendering
happens.
@lencioni
Copy link
Contributor Author

lencioni commented Feb 3, 2017

@majapw Everything is updated. PTAL!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 86.501% when pulling 91f1ae4 on avoid-rendering-when-closed into 1deaa6c on master.

@lencioni lencioni mentioned this pull request Feb 3, 2017
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.

None yet

3 participants