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

Get default value for closeWidgets from props #28

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tn3rb
Copy link

@tn3rb tn3rb commented Jul 20, 2020

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.

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.

1 participant