Passing functions to onClick

[sankomil]

I have created a dropdown meny with an Image acting like the trigger like so:

    <Image
          alt="Plus"
          aria-controls="long-menu"
          className={classes.plus}
          src={Plus}
          {...bindTrigger(popupState)}
          // onClick={handleClick}
        />

I want to pass a function called handleClick to onClick, but if I remove the comment then it overrides the onClick function generated by bindtrigger. I wanted to ask if there is a way to pass handleClick to the generated onClick?

[jedwards1211]

Yeah currently there isn't a convenient way to handle this.

If you need to preventDefault or stopPropagation, there might be a nearby ancestor element you could do so on. Otherwise, right now you'll have to do

onClick={e => {
  // perform additional action...and then:
  bindTrigger(popupState).onClick(e)
}}

If you just need to perform an additional effect when the popup is opened, and you're using a functional component, I would recommend performing an effect when popupState.isOpen changes:

React.useEffect(() => {
  if (popupState.isOpen) {
    // popup was just opened, do something
    // popupState.anchorEl is the element that was clicked
  }
}, [popupState.isOpen])

This is better since if you later need to add additional ways the popup can be opened, all of them will trigger this effect, whereas an onClick handler wouldn't respond to the new ways of opening the popup.

There are several ways I could make the API support this, none I'm 100% happy with, each one is awkward in some way.
Also, does the popup get opened before or after your onClick handler? Some people might want before, some after, and adding an option to control that would make these examples even more bulky. At least if you do the wrapping yourself, as in my example above, you have complete freedom to control this.

Possibility 1

{bindTrigger(
  popupState,
  <Image
    alt="Plus"
    onClick={handleClick}
  />
)}

Possibility 2

<Image
  alt="Plus"
  {...bindTrigger(popupState, {
    onClick: handleClick,   
  })}
/>

Possibility 3

// at root level:
const TriggerImage = createTrigger(Image)

// somewhere later:
<TriggerImage
  popupState={popupState}
  alt="Plus"
  onClick={handleClick}
/>

[sankomil]

Yes, I found that running handleClick when the menu is open works for what I am trying to achieve! Thank you for your help!

[jedwards1211]

Okay great! I'm still thinking about maybe releasing some kind of API for merging user event handlers with the injected ones. I haven't decided yet...

[sankomil]

Cool, all the best with that! I'll close the issue now.

[ifndefdeadmau5]

Yeah currently there isn't a convenient way to handle this.

If you need to preventDefault or stopPropagation, there might be a nearby ancestor element you could do so on. Otherwise, right now you'll have to do

onClick={e => {
  // perform additional action...and then:
  bindTrigger(popupState).onClick(e)
}}

If you just need to perform an additional effect when the popup is opened, and you're using a functional component, I would recommend performing an effect when popupState.isOpen changes:

React.useEffect(() => {
  if (popupState.isOpen) {
    // popup was just opened, do something
    // popupState.anchorEl is the element that was clicked
  }
}, [popupState.isOpen])

This is better since if you later need to add additional ways the popup can be opened, all of them will trigger this effect, whereas an onClick handler wouldn't respond to the new ways of opening the popup.

There are several ways I could make the API support this, none I'm 100% happy with, each one is awkward in some way. Also, does the popup get opened before or after your onClick handler? Some people might want before, some after, and adding an option to control that would make these examples even more bulky. At least if you do the wrapping yourself, as in my example above, you have complete freedom to control this.

Possibility 1

{bindTrigger(
  popupState,
  <Image
    alt="Plus"
    onClick={handleClick}
  />
)}

Possibility 2

<Image
  alt="Plus"
  {...bindTrigger(popupState, {
    onClick: handleClick,   
  })}
/>

Possibility 3

// at root level:
const TriggerImage = createTrigger(Image)

// somewhere later:
<TriggerImage
  popupState={popupState}
  alt="Plus"
  onClick={handleClick}
/>

Possibility 2 is exactly what I imagined before checking out this issue, which seems pretty straightforward to me. Could this be implemented somehow? I can work on it if you want some help.

[jedwards1211]

The main problem with possibility 2 is I can't see any good way to memoize the combined onClick function. It would have to return a new function each time, which would force the receiving component to rerender in most cases.

If you combine them yourself you can at least choose to do so in a useCallback earlier in function components, etc.

But I guess we could implement option 2 and just warn in docs that it's always going to return a new onClick function. I feel on the fence about it, what do you think?

[jedwards1211]

Actually let me think about it some more. i guess we could store the previous username onClick in a ref in the popup state and use that to memoize. But I'd need to make the <PopupState> component use hooks internally and drop support for React pre-hooks.

[andrew2764]

I figured sort of a workaround by augmenting the internal methods in popupState. In this case, when I click on the button to toggle the popup, it internally calls toggle, so it kind of acts as an onClick where we have control over whether to perform actions before or after the popup opens. What do you think of this approach? Or am I unintentionally doing something I'm not supposed to do?

const popupState = usePopupState({ popupId, variant: "popper" });
const modifiedPopupState = {
    ...popupState,
    toggle: (arguments_) => {
      // Do stuff
      popupState.toggle(arguments_);
    },
  };

// somewhere later
return (
  <div>
    <button {...bindToggle(modifiedPopupState)}>Open</button>
    <BasePopper {...bindPopover(modifiedPopupState)}>
      <div>
        <h1>Lorem Ipsum</h1>
      </div>
    </BasePopper>
  </div>
);

[jedwards1211]

@andrew2764 that wouldn't currently work because open, close, etc call the original toggle function directly, not whatever one is attached to the modified popup state. But this makes me think, I should try to change the code to where things like this work, whether or not it's recommended.

[jedwards1211]

@ifndefdeadmau5 @andrew2764 I've been thinking about this some more...the main problem with all three possibilities above is, does the custom onClick logic run before or after the bindTrigger logic? It could go either way...I can imagine an argument between camps of users who think it should run before and who think it should run after.

So now I'm leaning toward an API with beforeX/afterX handlers like

<Image
  alt="Plus"
  {...bindTrigger(popupState, {
    beforeClick: handleClick,   
  })}
/>

[jedwards1211]

🎉 This issue has been resolved in version 5.1.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[jedwards1211]

Okay I decided to go with an API like

<Image
  alt="Plus"
  {...chainEventHandlers(
    bindTrigger(popupState),
    { onClick: handleClick },   
  )}
/>

This makes specifying the order of the handlers straightforward and would be useful for combining multiple bindTriggers etc.

[andrew2764]

Thanks! I ended up using the approach from the sample code in the README

const button = (
  <Button
    {...bindTrigger(popupState)}
    onClick={(e: React.MouseEvent) => {
      performCustomAction(e)
      bindTrigger(popupState).onClick(e)
    }}
  >
    Open Menu
  </Button>
)

The only difference is that I needed to perform an action before the bindTrigger logic.

[jedwards1211]

@andrew2764 yeah personally I still prefer that approach, the control flow would be more straightforward in a debugger