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

Keep scroll position when prev months rendered on vertical scrollable calendar #1902

Merged

Conversation

nkinser
Copy link
Contributor

@nkinser nkinser commented Jan 15, 2020

Keep track of the scroll position so that the visible month stays in view when prev months are rendered for the vertical scrollable calendar.

Before

wrong-cal-position

After

correct-cal-position

@nkinser nkinser force-pushed the nk--vertical-scrollable-cal-month-position branch from c473c48 to d5b4fef Compare January 15, 2020 04:57
@nkinser nkinser requested a review from ljharb January 15, 2020 05:02
@nkinser nkinser force-pushed the nk--vertical-scrollable-cal-month-position branch from d5b4fef to c1c2e22 Compare January 15, 2020 05:04
@nkinser
Copy link
Contributor Author

nkinser commented Jan 15, 2020

@ljharb I'm failing the coverage check but most of the lines highlighted as untested are not additions by me in this PR. Any suggestions?

@ljharb
Copy link
Member

ljharb commented Jan 15, 2020

If every line you've added is fully covered - every branch - then this means you've added enough lines to change the fraction, which makes coverage decrease. The desired effect is to force you to add tests to cover the preexisting uncovered lines.

However, it's fine to merge through as well; it'll just put some future PR author in the same boat.

const {
orientation, daySize, isFocused, numberOfMonths,
} = this.props;
const { focusedDate, monthTitleHeight } = this.state;
const {
currentMonth, currentMonthScrollTop, focusedDate, monthTitleHeight,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
currentMonth, currentMonthScrollTop, focusedDate, monthTitleHeight,
currentMonth,
currentMonthScrollTop,
focusedDate,
monthTitleHeight,

wrapper.setState({
currentMonth: moment().subtract(1, 'months'),
});
expect(wrapper.transitionContainer.scrollTop).to.not.equal(prevScrollTop);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(wrapper.transitionContainer.scrollTop).to.not.equal(prevScrollTop);
expect(wrapper.transitionContainer).to.not.have.property(’scrollTop’, prevScrollTop);

@ljharb ljharb added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Jan 15, 2020
@nkinser nkinser force-pushed the nk--vertical-scrollable-cal-month-position branch 4 times, most recently from 44cfccf to dd091ec Compare January 15, 2020 19:02
@nkinser nkinser force-pushed the nk--vertical-scrollable-cal-month-position branch from dd091ec to 399ba3c Compare January 15, 2020 19:06
@nkinser nkinser merged commit b9ddf40 into react-dates:master Jan 15, 2020
@nkinser nkinser deleted the nk--vertical-scrollable-cal-month-position branch January 15, 2020 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants