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

Pass isVisible to renderMonthElement #1609

Merged
merged 1 commit into from Apr 8, 2019

Conversation

Projects
None yet
3 participants
@gidztech
Copy link
Contributor

gidztech commented Apr 6, 2019

renderMonthElement is called multiple times when you navigate between months. This is because the month before and the month after the currently visible month are still rendered to the DOM, but are hidden with CSS. This is presumably done for smoother animation/performance.

However, it would be very useful to know which month is actually visible when renders occur. This PR passes through the isVisible prop received in the CalendarMonth component to the renderMonthElement event.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 6, 2019

Coverage Status

Coverage increased (+0.2%) to 84.494% when pulling 5f1964b on gidztech:render_month_is_visible into 28ce831 on airbnb:master.

@ljharb
Copy link
Member

ljharb left a comment

Can you add tests that cover this?

@gidztech gidztech force-pushed the gidztech:render_month_is_visible branch from 665c36c to db54efd Apr 8, 2019

@gidztech

This comment has been minimized.

Copy link
Contributor Author

gidztech commented Apr 8, 2019

@ljharb

I'm struggling to add a test for this. Firstly, I'm not sure what test is relevant here. There doesn't appear to be any tests of the renderMonthElement prop currently. I tried to write a test that would check the type of arguments received in the renderMonthElement handler after artificially invoking onPrevMonthClick , but I don't think it works without using Enzyme's mount with JSDom. I'm also not sure that that test actually provides much value here.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 8, 2019

It seems like a shallow test that passes in a sinon spy as renderMonthElement would be able to assert on the arguments it receives, and also assert that the spy's return value gets rendered?

@gidztech

This comment has been minimized.

Copy link
Contributor Author

gidztech commented Apr 8, 2019

@ljharb I had already tried that but found that the renderMonthElement function wasn't being called. The call count was always 0 on the spy. I'm guessing it has to do with Enzyme's shallow rendering?

const renderMonthElementSpy = sinon.spy();
const wrapper = shallow(<DayPicker renderMonthElement={renderMonthElementSpy} />).dive();
wrapper.instance().onPrevMonthClick();
expect(renderMonthElementSpy.callCount).to.equal(1);
@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 8, 2019

It should be called by a shallow test on CalendarMonth directly

Show resolved Hide resolved test/components/CalendarMonth_spec.jsx Outdated
Show resolved Hide resolved src/components/CalendarMonth.jsx Outdated
Show resolved Hide resolved test/components/CalendarMonth_spec.jsx Outdated

@gidztech gidztech force-pushed the gidztech:render_month_is_visible branch 2 times, most recently from c92f0b8 to 1924094 Apr 8, 2019

@gidztech

This comment has been minimized.

Copy link
Contributor Author

gidztech commented Apr 8, 2019

@ljharb Should be good for review now.

@ljharb ljharb force-pushed the gidztech:render_month_is_visible branch 2 times, most recently from c355e9b to 5f1964b Apr 8, 2019

@ljharb

ljharb approved these changes Apr 8, 2019

@ljharb ljharb merged commit 5f1964b into airbnb:master Apr 8, 2019

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 84.494%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.