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.

Sign up for GitHub

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

Jump to bottom

Add vertical scrollable orientation #250

Merged
merged 1 commit into from
Jan 27, 2017
Merged

Add vertical scrollable orientation #250

merged 1 commit into from
Jan 27, 2017
+196 −32

Conversation

moonboots
Copy link
Collaborator

to: @majapw

Adds a new orientation that makes DayPicker vertically scrollable instead of using pagination. This scrollable DayPicker will fill the height of its parent and show numberOfMonths.

Replaces this previous PR. I've separated css by component and threaded the new orientation property throughout.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 87.356% when pulling 3eba56f on moonboots:vertical-scrollable into f931a82 on airbnb:master.

@moonboots moonboots force-pushed the vertical-scrollable branch 2 times, most recently from 06cdd62 to bbbdd20 Compare January 25, 2017 22:15
@moonboots
Copy link
Collaborator Author

@majapw I've update this PR with a new PropType shape ScrollableOrientationShape that's used by internal classes. Top level components that don't support scroll like DateRangePicker continue to use OrientationShape, which will guard against passing in unsupported orientations.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 87.429% when pulling bbbdd20 on moonboots:vertical-scrollable into 1cbc4db on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 87.429% when pulling bbbdd20 on moonboots:vertical-scrollable into 1cbc4db on airbnb:master.

@moonboots moonboots force-pushed the vertical-scrollable branch from bbbdd20 to c06e35e Compare January 26, 2017 00:20
majapw
majapw requested changes Jan 26, 2017
View reviewed changes
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.

A handful of comments, but looking good overall.

css/CalendarMonth.scss
@@ -35,6 +35,10 @@
text-align: center;
// necessary to not hide borders in FF
margin-bottom: 2px;

.CalendarMonth--vertical-scrollable & {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this is not terribly clear? I'd rather have

.CalendarMonth--horizontal.CalendarMonth__caption,
.CalendarMonth--vertical.CalendarMonth__caption {
  padding: 15px 0 35px;
}

.CalendarMonth--vertical-scrollable.CalendarMonth__caption {
  padding: 5px 0;
}

src/components/DayPicker.jsx
} = this.props;

const onNextMonthClick = orientation === VERTICAL_SCROLLABLE
? this.multiplyScrollableMonths
: this.onNextMonthClic;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this.onNextMonthClick, you're missing a k.

Also we prefer an if... then over a multiline ternary.

src/components/DayPicker.jsx
const dayPickerClassNames = cx('DayPicker', {
'DayPicker--horizontal': this.isHorizontal(),
'DayPicker--vertical': this.isVertical(),
'DayPicker--vertical-scrollable': this.props.orientation === VERTICAL_SCROLLABLE,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume you mean to be reused the verticalScrollable from line 404

stories/DateRangePicker.js
ANCHOR_RIGHT,
VERTICAL_ORIENTATION,
VERTICAL_SCROLLABLE,
} from '../constants';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to make this addition?

src/components/DayPickerRangeController.jsx
@@ -36,10 +36,11 @@ const propTypes = {
// DayPicker props
enableOutsideDays: PropTypes.bool,
numberOfMonths: PropTypes.number,
orientation: OrientationShape,
orientation: ScrollableOrientationShape,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this correct?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 85.04% when pulling c06e35e on moonboots:vertical-scrollable into fa4d785 on airbnb:master.

@moonboots moonboots force-pushed the vertical-scrollable branch from c06e35e to b049184 Compare January 26, 2017 01:05
@moonboots
Copy link
Collaborator Author

Thanks @majapw, fixed up the PR. The orientation changes in DayPickerRangeController allow a custom DayRangePicker to pass the scrollable orientation to the underlying DayPicker. There were a few leftover scrollable props from my previous PR that I've removed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 85.141% when pulling b049184 on moonboots:vertical-scrollable into 67c0fb5 on airbnb:master.

@moonboots moonboots force-pushed the vertical-scrollable branch from b049184 to ff7ccc8 Compare January 26, 2017 22:14
@moonboots moonboots force-pushed the vertical-scrollable branch from ff7ccc8 to 72d0b1f Compare January 26, 2017 22:25
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 72d0b1f on moonboots:vertical-scrollable into ** on airbnb:master**.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 85.141% when pulling 72d0b1f on moonboots:vertical-scrollable into 2011965 on airbnb:master.

majapw
majapw approved these changes Jan 27, 2017
View reviewed changes
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.

This looks great to me! Will merge in and do a release ASAP.

Also I'm thinking it'd be good to have some stories for the DayPickerRangeController

@majapw majapw added the semver-minor: new stuff Any feature or API addition. label Jan 27, 2017
@majapw majapw merged commit c579349 into react-dates:master Jan 27, 2017
@oyeanuj
Copy link

oyeanuj commented May 21, 2017

@majapw @moonboots Curious if there was a reason that this orientation wasn't included for SingleDatePicker?

In dealing with responsive decisions, this might be a much useful option.

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-minor: new stuff Any feature or API addition.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@moonboots @coveralls @oyeanuj @majapw