fix(Popup): call onOpenChange on all user actions

[levithomason]

The Popup currently does not call onOpenChange when clicking on the trigger if there is also an open prop provided. This makes it impossible to control the Popup in the expected way:

handleOpenChange = (e, { open }) => this.setState({ open })

<Popup
  open={this.state.open}
  trigger={<button>Toggle</button>} 
  onOpenChange={this.handleOpenChange}
/>

This was because the Popup was only calling onOpenChange if the Popup successfully controlled its own state. Now, it works. We always call the change handler when the an end user event attempts to change the open value.

[kuzhelov]

this naming is used in lots of files (private vs public), and it doesn't provide any harm. More than that, occasionally it was helpful to folks to differentiate React methods from custom ones of the component.

I don't think that we should change this in that PR, especially if we are talking about single component file - it should be done either for all or not done at all (we've already had related discussions and agreed to proceed with explicit names for visibility scopes), but in either case it shouldn't be a matter of change for this PR

[levithomason]

OK, public/private make no sense in a React class. I will address it separately.

[kuzhelov]

agree with changing trigger and content semantics for Children API, but disagree with the current onOpenChange changes - note that currently Controlled Component example is broken,.

Also, note that implementation should ensure that the following semantics should be preserved: in case if controlled prop is changed, event shouldn't be fired. Please, take a look on the similar example where React input component is operating in controlled mode: https://codesandbox.io/s/oqvpj5k945

[levithomason]

agree with changing trigger and content semantics for Children API

👍 Removed for now, it will be done separately. It turned into a rabbit hole of other updates unrelated to the fix here.

Also, note that implementation should ensure that the following semantics should be preserved: in case if controlled prop is changed, event shouldn't be fired. Please, take a look on the similar example where React input component is operating in controlled mode: https://codesandbox.io/s/oqvpj5k945

😕 Hm, this bug doesn't exist in this PR, so I am a bit confused why it is raised so strongly?

Food for thought, giving bold instructions and requesting study of samples you've prepared for me communicates an awkward sense of superiority. There is an assumption here that you have some special knowledge that I need to pay very close attention to and that I need to carefully study your work in order to understand it.

If I had demonstrated a lack of knowledge or skill then the advice would be much welcomed 🙇. However, the assumption of superiority is a bit patronizing. No ill intent in my response here. I hope the honesty is well received ❤️

[kuzhelov]

@levithomason

these "bold instructions" were just to emphasize the behavior I would like to be preserved with the proposed changes - there are no instructions by any means, but just a way to emphasize a key statement from the paragraph. This invariant (captured by these words) was discussed several times and we had always found it to be a subtle thing to ensure, thus this point was reminded. As you might recall, we've even discussed what we can do to ensure this thing on a regular sync meeting, and agreed to introduce tests. And I am pretty sure that I am not the only person who shares concerns in PR comments and uses double stars.

With that being said, let me refrain from sharing my emotions in public, apologise and promise that I won't use any emphasis in the comments for your PR anymore - because, apparently, this may lead to serious misunderstanding