react-datetimerange-picker icon indicating copy to clipboard operation
react-datetimerange-picker copied to clipboard

Get default value for closeWidgets from props

Open tn3rb opened this issue 5 years ago • 0 comments

First off, thank you so much for an amazing suite of date and time picker components you have built.

This PR provides a fix for a super minor bug I found while using react-datetimerange-picker in a project.

In DateTimeRangePicker.onDateChange() the default value for the closeWidgets parameter is hardcoded to true:

onDateChange = ([valueFrom, valueTo], closeWidgets = true) => {

this negates the value of the closeWidgets prop passed to the component, since that prop value is never passed to onDateChange() (on line 367, the only place the method is called):

          <Calendar
            className={calendarClassName}
            onChange={this.onDateChange}
            selectRange
            value={value || null}
            {...calendarProps}
          />

So passing closeWidgets={false} to the component still results in the widget closing whenever the date changes.

This PR simply changes that hardcoded value for closeWidgets to use the value already in the class props:

  onDateChange = ([valueFrom, valueTo], closeWidgets = this.props.closeWidgets) => {

alternate approaches

if you don't like the above solution then here are some other ways to fix this issue that I would be happy to implement

  1. removing parameter and deconstructing closeWidgets from props:
  // in renderCalendar() 

  onDateChange = ([valueFrom, valueTo]) => {
    const { closeWidgets, value } = this.props;

this may be acceptable because the only place this method is called within the library is from the calendar, which can't possibly pass that prop as is, since it has no access to the datetime range picker's internal props. This unfortunately would not be backwards compatible with any third party code that is utilizing DateTimeRangePicker.onDateChange() in an external app.

  1. creating a wrapper function for onDateChange() prior to passing off to the calendar that receives the new date values and then supplies the value for closeWidgets from props:
    // in renderCalendar() 

	const onDateChange = (values) => {
      this.onDateChange(values, this.props.closeWidgets)
	}

    return (
      <Fit>
        <div className={mergeClassNames(className, `${className}--${isCalendarOpen ? 'open' : 'closed'}`)}>
          <Calendar
            className={calendarClassName}
            onChange={onDateChange} // <<< using new wrapper function
            selectRange
            value={value || null}
            {...calendarProps}
          />
        </div>
      </Fit>
    );

So again, would be more than happy to implement one of these other approaches if you prefer one of them over the fix in this PR.

Looking forward to hearing back from you.

Thank you for your time, effort, and talent.

tn3rb avatar Jul 20 '20 18:07 tn3rb