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

Add DayPickerSingleDatePicker component #573

Merged
merged 1 commit into from
Jun 28, 2017

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Jun 16, 2017

Fix for #524

I've created a DayPickerSingleDateController to match the DayPickerRangeController component. I've also added stories for it as well.

The related PR for ranges is #167. The basic gist of this work is that all calendar related code has been moved into an abstraction layer in between the DayPicker and the SingleDatePicker called the DayPickerSingleDateController. Specifically this includes the day interaction callbacks (onDayMouseEnter, onDayMouseLeave, and onDayClick) and all code relating to the modifiers.

The following graphic should help explain the new structure somewhat (DayPickerWithRangeControllers maps to the DayPickerSingleDateController).
new SDP structure

In a future PR, I will separate out the code related strictly to the input from the SingleDatePicker. After that the SingleDatePicker will strictly contain code that coordinates focus and visibility between the input and the calendar dropdown.

to: @tagoro9 @pokonski @airbnb/webinfra

@majapw majapw added the semver-minor: new stuff Any feature or API addition. label Jun 16, 2017
@pokonski
Copy link

Fantastic news! I'll check it out very soon, thanks Maja 👏

@tagoro9
Copy link
Contributor

tagoro9 commented Jun 16, 2017

This is awesome! Thanks so much!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 86.162% when pulling 100d80c on maja-add-daypickersingledatecontroller into 0c14a9d on master.

@majapw
Copy link
Collaborator Author

majapw commented Jun 17, 2017

@ljharb I've added a lot more content to the description to provide context for the changes if you would like to take a look! It does largely just entail moving around a functions that already existed and I do have confidence in the tests catching things. :D

ljharb
ljharb previously approved these changes Jun 17, 2017
@tagoro9
Copy link
Contributor

tagoro9 commented Jun 21, 2017

I just switched to this version in our library and replaced the uses of our custom implementation of DayPickerSingleDateController with the one in here and everything is working great and test are passing. This is awesome! Thank you very much!

I just have one question. DayPickerSingleDateController is not exported in the index.js file. Are you planning on exporting it?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 86.162% when pulling 5275629 on maja-add-daypickersingledatecontroller into 61f505c on master.

@majapw majapw force-pushed the maja-add-daypickersingledatecontroller branch from 5275629 to 7d173c1 Compare June 27, 2017 23:07
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 86.162% when pulling 7d173c1 on maja-add-daypickersingledatecontroller into 61f505c on master.

@majapw majapw force-pushed the maja-add-daypickersingledatecontroller branch from 7d173c1 to 730acf8 Compare June 27, 2017 23:45
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 86.173% when pulling 730acf8 on maja-add-daypickersingledatecontroller into 61f505c on master.

@majapw majapw force-pushed the maja-add-daypickersingledatecontroller branch from 730acf8 to 229579f Compare June 28, 2017 00:13
@majapw majapw force-pushed the maja-add-daypickersingledatecontroller branch from 229579f to 48dfbd1 Compare June 28, 2017 00:33
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM overall

}

componentDidMount() {
this.isTouchDevice = isTouchDevice();
Copy link
Member

Choose a reason for hiding this comment

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

should this trigger a rerender, if it changes?

}

getModifiersForDay(day) {
return new Set(Object.keys(this.modifiers).filter(modifier => this.modifiers[modifier](day)));
Copy link
Member

Choose a reason for hiding this comment

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

should this be new Set(Object.values(this.modifiers).map(x => x(day)))? (using object.values ofc)

(fine as-is, too)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 86.204% when pulling 48dfbd1 on maja-add-daypickersingledatecontroller into 24bd840 on master.

@majapw majapw merged commit ddb6e32 into master Jun 28, 2017
@majapw majapw deleted the maja-add-daypickersingledatecontroller branch June 28, 2017 00:58
Copy link
Collaborator

@timhwang21 timhwang21 left a comment

Choose a reason for hiding this comment

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

values(visibleDays).forEach((days) => {
Object.keys(days).forEach((day) => {
const momentObj = moment(day);
if (isDayBlocked(momentObj)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@majapw I think this lost the change fixed in #550. Specifically, checks for isBlocked and isOutsideRange. Cheers!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoooops. Man, I knew I should have rebased. :|

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I think this is back now and released in 12.2.2. D:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor: new stuff Any feature or API addition.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants