Added a way to open / close the calendar outside of the datepicker component

[GBT3101]

Hi,
I work at Wix, and we needed a way to open / close the calendar outside of the component (for example, opening the calendar with an external button).

I've done that by adding 2 new props: "isOpen" (boolean) and "setOpen" (function).
This functionality will only work by adding these props, and will not affect the object in any way if those props were not added.

isOpen - will live alongside the internal ‘state.open’ in correlation.
setOpen - is a callback that will be called if the function is defined and the state has been changed.

I've also added a simple example that shows a connection between the calendar and a checkbox using this new feature.

Hope you will like it, have a nice week.
approved by @lbelinsk

[GBT3101]

The test fails with / without my commit, what can I do about it?

[aij]

Would you mind making it clearer that this is adding another way to open/close the component from outside? (It can already be set statefully via the the .setOpen() method.)

I'm all for allowing the state to be fully controlled via props, but in that case, the prop should always take precedence if it is specified. Otherwise we end up with confusing behavior, like the datepicker closes itself despite having isOpen={true}.

IE, we should use the value of the prop instead of treating it as an edge-trigger to toggle internal state.

[lbelinsk]

@aij Since there are a lot of users for this library, we didn't want to break any existing behavior.
We have 3 interesting events:

1. constructor: state.open will be initialized by the props.isOpen value (which is false by default).
2. user did focus / blur: this.setOpen will take care of the state and will call the props.setOpen callback to allow the user to change the props.isOpen.
3. User passed a different props.isOpen: We handle this case in componentWillReceiveProps and call this.setOpen.

So in each case, we make sure the state and the prop are in correlation. The "react way" will be to delete the state.open boolean, and to work only with the props.isOpen, but we should remember to have backward compatibility.

This is our solution, but there is an alternative one, like you suggested, to completely ignore the state.open if the props.isOpen was given.

I personally like the first solution better, but we are open for discussion.

[aij]

@lbelinsk When we call props.setOpen, and the user chooses not to change props.isOpen, how do we make sure the prop and the state are in correlation? It seems to me like we don't.

[rafeememon]

I agree with @aij; to summarize the requirements:

• If the prop is not specified, we need to maintain the existing behavior of relying on component state for backwards compatibility
• If the prop is specified, it must override the state so that it always correlates to the observed state of the picker

[GBT3101]

I understand what you mean with the confusing behaviour, theoretically props.setOpen callback is used so the developer will change his external isOpen value, as shown in the example (the openCalendar function used to keep this correlation).
I've also made sure to expose this code in the example's description, so developers will understand how to use this callback.
the callback is triggered every time the internal "state.open" is being changed, so as long the developer chooses to keep the correlation it's his responsibility.

That said, it's only to explain our approach about the problematic behaviour you described.
Ultimately our first priority is to enable closing & opening the calendar from outside the component + not to harm in any way developers that are already using the component, your solution also provides this goal but here are some drawbacks I think you should consider:

1. In your suggested solution, where we will treat the prop.isOpen as an override over the state.open boolean, we can’t completely stop updating the internal state. even If I as a user want to be able to control the visibility of the calendar with a boolean prop, I would expect the component to close and open on focus and blur, without me doing anything from outside.

2. Future contributors will have to maintain 2 scenarios:
   - the current one, calendar visibility fully controlled from the internal state.
   - the alternative, a prop that is being practically used as a state (a function inside the
   component, handleOpenChange that will change the prop).

The bigger the component will get, the harder it will be to maintain that 2 scenarios, I think in the long run it will cause that different developed attributes will work great alone, but might not work too well with different combinations because developers will overlook this new open-state-prop mutation.

After considering this and the example with the checkbox (that teach developers how to use the callback appropriately and maintain correlation between the prop.isOpen and state.open), advise which solution you prefer and I will change it accordingly.

Thanks.

[rafeememon]

If I as a user want to be able to control the visibility of the calendar with a boolean prop, I would expect the component to close and open on focus and blur, without me doing anything from outside.

I disagree with this -- if you choose to override the default behavior, you are giving up the automatic handling of open/close state. You can decide how to update the isOpen prop via the onChange or various other existing callbacks. Overriding the state when the prop is specified is much easier to understand than expecting a consumer to keep themselves in sync with the picker's state, rather than the other way around.

[lbelinsk]

As @GBT3101 said before, we will go with the approach you will see best.
I care enough about this component, so I will just raise some final concerns:

1. The user would have to pass onFocus as props. This callback will have to modify the external state. It will be called here:
   https://github.com/Hacker0x01/react-datepicker/blob/master/src/datepicker.jsx#L176-L180

2. The user would have to pass onBlur as props. This callback will have to modify the external state. It will be called here:
   https://github.com/Hacker0x01/react-datepicker/blob/master/src/datepicker.jsx#L197-L202

IMHO, It feels like the user has too much control in his hands like he has 2 separated components, Calendar popup and an input which he needs to sync together from scratch.

Other than that, the user can decide to modify the external state with "onSelect" or "onChange" callbacks, which is fine.

Please let us know what is the final solution. We will make sure to implement it tomorrow according to your decision.

Thanks for the quick responses, it is really appreciated.

[micahalcorn]

Possibly related question:

In my experience, the calendar closes by default when a date is selected. When showTimeSelect is true, the calendar does not close when a time is selected. The current default workflow is actually perfect if a user selects a time first and then a date, but that isn't necessarily intuitive. So it's nice to have @lbelinsk's shouldCloseOnSelect (#992) solution to prevent any automatic closing. Would it be appropriate to have a more granular closing controls for when to internally close the calendar? Or does the solution proposed here sufficiently solve my case by passing the auto-close responsibility to the developer?

[lbelinsk]

@micahalcorn we have discussed about 2 possible solutions:

1. Keep the current "uncontrolled" behavior but also allow add isOpen and setOpen props that would allow you to control the state of the calendar and keeping the state.open and props.isOpen synced. It means that as long as you won't provide these new props then everything will work normally, but when you do provide the new props then the control is in your hands by implementing the callback function to hide or show the calendar. It will update the inner state of the component as well. You won't have to take care of the obvious behavior of showing and hiding the calendar on focus and blur respectively.

2. The solution, which sounds like we are going to implement soon since the first approach was not approved here, is making the component support 2 modes: controlled and uncontrolled. It means that by passing the new props the component will no longer work with its internal state.open, and instead will only listen to the new prop which would get updated by the callback the user will pass.
   So the user would have to also take care of onFocus and onBlur events to hide and close the calendar. so you basically going to get all or nothing.

@rafeememon @aij can you confirm that the desired solution is the second one?
@GBT3101 and I will take care of it hopefully this week (we had a lot of holidays here during the past few weeks :))

[aij]

@lbelinsk I was thinking closer to 1, but making it so props.isOpen would actually allow you to control the state of the calendar. So the example in examples/set_open.jsx would stay the same, except that the opening and closing of the calendar would happen in response to the value of props.isOpen rather than in spite of it.

TBH, I'm not sure how useful it is to control the openness of the calendar via props. It seems kind of akin to controlling focus via props, which React has decided not to do. It's not quite the same, because it never makes sense to have two inputs have focus at the same time but it might make sense to have more than one datepicker render as open at the same time, though I expect that would require other code changes as we've made assumptions that kind of tied calendar openness to focus.

I think we have some confusion in this discussion due to the original comment (and commit message) being slightly misleading. (See my first comment where I requested that be clarified.)

In case it wasn't clear enough earlier, the datepicker can already be opened and closed from outside the component, by calling .setOpen(true||false) on the instance of the datepicker that you want to open or close. (Much like focus on a regular input can be set by calling .focus() on it.)

Of course, calling .setOpen() is kind of fire-and-forget. It lets the user set the current open state, but not take full control over it. (Since the calendar continues to open/close as usual in response to clicks/focus/etc.)

Perhaps another source of confusion is that this PR is adding a new setOpen prop, with the same name as the existing .setOpen() method. (Above, I was referring to the existing method, not the new prop.) Perhaps if we do keep the new callback prop it should be named onSetOpen to avoid confusion. (Much like React has .focus() and onFocus.)

That said, I still don't understand the motivation for this PR (other than perhaps misunderstanding what is already possible -- which is hopefully clearer now).

Is it just about being able to open or close the calendar externally? Then perhaps the new example could be updated to use the existing API.

Does someone need callbacks in order to respond to the opening/closing of the calendar? Then it seems the new setOpen prop is all that's needed (though I'd still prefer it be renamed to avoid confusion).

Does someone need to take control over when the calendar opens or closes? Then we could add the isOpen prop, but we should actually honor it.

Perhaps an example might help. This page shows how we are using react-datepicker at CCAP: https://wccabeta.wicourts.gov/lawEnforcementCalendar.html Note how clicking the little calendar buttons opens and focuses the corresponding datepicker. That is of course already possible.

Now could someone elaborate on what it is that Wix is trying to do? (And perhaps how that is different from what the existing API allows?)

[lbelinsk]

@aij thanks for your response.
I will start from the beginning in order to explain our motivation. We needed to allow opening and closing the calendar not by focusing on the input (that's all :)).

Use case:
Actually, the CCAP example is pretty similar to out usage.
A form which has a checkbox "Today" and if you uncheck it then you will be requested to insert some other date. By unchecking this checkbox, a date-picker is being rendered and we wanted it to be rendered with an open calendar. So basically the "onClick" of the checkbox should open the calendar.

We implemented it on our side by placing ref prop on the react-date-picker and calling ref.setOpen(true||false). Of course It worked, but we felt maybe this is not exactly the "react way". In React the data flow is from top to bottom, and this approach kind of violates it.
We have a closed pr for that and I can share a link if it will be relevant.

Regarding this comment:

@lbelinsk I was thinking closer to 1, but making it so props.isOpen would actually allow you to control the state of the calendar. So the example in examples/set_open.jsx would stay the same, except that the opening and closing of the calendar would happen in response to the value of props.isOpen rather than in spite of it.

We thought about it in the beginning, but then we understood that there is one problem with this solution. Let's say we have a closed calendar. Now let's change props.isOpen to be true, and the calendar will be visible. Until here all good. Now let's say I clicked outside of the input, so now it got closed. Notice that the props.isOpen is still true, although the calendar is hidden. Now in order to open the calendar again with this prop, you can't really do anything since it's value is already true. You can change it to false and then again to true and this is the only way to open the calendar. This is not a nice solution. This is why we think the props.setOpen is needed. We can rename it of course.

@aij I completely agree with you that it is similar to ref.focus() of input, but we try to avoid using it in general.
This is why we thought it will be nice to enrich the component with "controlled calendar".
Having read your comment, I am convinced that the option of using the ref.setOpen() is better than solution #2 that was suggested here. We would still prefer having our #1 suggestion, although it creates some confusion in the component's behavior and adds some complexity to the state management.

Having said that, You guys are the watchers over the wall, and if this solution is not the one you want to go with then let's just roll back to the existing solution ref.setOpen().
We can update only an example of examples/set_open.jsx and not to add any further changes.

[lbelinsk]

Hey, so after this long conversation, what solution are we going to pursue?

1. ref.setOpen(true||false) - We can add a usage example, but no code change is needed.
2. props.isOpen and props.setOpen

[rafeememon]

Let's go with (1) -- as you called out, the other solution adds confusion to the behavior and seems like one step forward and two steps backward. IMO, the isOpen prop is only viable if it is honored 100% of the time.

[lbelinsk]

Cool, so number 1 it is...