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

Enable disabling of certain dates #221

Merged
merged 6 commits into from
Aug 7, 2018
Merged

Conversation

thijssteel
Copy link
Contributor

@thijssteel thijssteel commented Jul 17, 2018

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description

  • Added a disabledDates props which holds an array of dates to be disabled. These days will be rendered with the disabled props.
  • Also handles the case when you would select a range that includes one of the disabled dates.
  • Added an example using this feature to the demo

Related Issue: #213

@thijssteel
Copy link
Contributor Author

i couldn't get the build to work however

Copy link
Collaborator

@mkg0 mkg0 left a comment

Choose a reason for hiding this comment

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

Can you add this to README.md and CHANGELOG.md?

@@ -72,6 +72,13 @@ export default class Main extends Component {
key: 'selection',
},
},
dateRangeWithDisabled: {
selection: {
startDate: new Date(new Date().getTime() + 24 * 60 * 60 * 1000),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use date utils

addDays(new Date(), 3)

@@ -116,7 +116,7 @@ class Calendar extends PureComponent {
setTimeout(() => this.focusToDate(this.state.focusedDate), 1);
}
}
componentWillReceiveProps(nextProps) {
UNSAFE_componentWillReceiveProps(nextProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will break compatibility of older react versions. It's better to remove for a while.

// reverse dates if startDate before endDate
let hasBeenReversed = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of keeping a bool for hasBeenReversed, can you expose a variable like below:

let isStartDateSelected = focusedRange[1] === 0;
// then
if (isBefore(endDate, startDate)) {
      isStartDateSelected = !isStartDateSelected;
      [startDate, endDate] = [endDate, startDate];	      [startDate, endDate] = [endDate, startDate];
    }	    
}

} else {
const maxDate = new Date(Math.max.apply(null, inValidDatesWithinRange));
endDate = new Date(maxDate.getTime() - 24 * 60 * 60 * 1000);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's can be cleaner:

if (isStartDateSelected) {
    startDate = addDays(max(disabledDates), 1);
} else {
    endDate = addDays(min(disabledDates), -1);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've noticed that this method has some problems. It fails when the first selected date is inbetween two invalid dates.

@@ -81,14 +102,14 @@ class DateRange extends Component {
this.props.onRangeFocusChange && this.props.onRangeFocusChange(focusedRange);
}
updatePreview(val) {
if (!val) {
if (!val || !val.wasValid) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should still show the preview because when a user clicks the date, It will select the date. Can you remove the preventing of preview disabling?

@thijssteel
Copy link
Contributor Author

should be ok now

@mkg0 mkg0 merged commit 2ffa7c8 into hypeserver:master Aug 7, 2018
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.

3 participants