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

Weird interaction with Portal.js and React.cloneElement #2195

Closed
mkarajohn opened this issue Oct 14, 2017 · 3 comments
Closed

Weird interaction with Portal.js and React.cloneElement #2195

mkarajohn opened this issue Oct 14, 2017 · 3 comments

Comments

@mkarajohn
Copy link
Contributor

mkarajohn commented Oct 14, 2017

DISCLAIMER: I am not sure if this is a bug on Semantic's end or on my end, that's why I am asking here for clarification.

Context

I have created a custom Modal component, not related to the Semantic UI one, because I needed some very custom behaviour.

However, I have retained a similar API to the semantic's Modal component, meaning my Modal takes a React element as a trigger to render, and as children the actual Modal's content

Now, inside my Modal's render method, because I need to call some of its own internal methods when the user clicks on the passed trigger element, I am doing a React.cloneElement() of the passed trigger and I return an enhancedTrigger element where I have overriden the original trigger element's onClick with a new onClick which also calls the original onClick. Something like this:

    const enhancedTrigger = trigger =>
      cloneElement(trigger, {
        onClick: this.handleTriggerClick // this is my Modal's instance 
      }); 

where this.handleTriggerClick() is

  handleTriggerClick(e) {
    // some code

    const { trigger } = this.props;

    _.invoke(trigger, 'props.onClick', e);
    this.setState({ open: true }, this.props.onOpen);
  }

My Modal simply renders the enhancedTrigger when not open and both an enhancedTrigger and the actual modal when it's open.

Now, I have a Semantic-UI Popup component, where I render a list of actions, one of which is my own Modal (remember, my Modal simply displays an enhancedTrigger element until it's clicked)

With this done, and my enhancedTrigger being rendered and shown in the DOM, when I click on the enhancedTrigger DOM node, the following thing happens, which makes no sense to me.

Inside Portal.js, lines 41-46

      if (!_this.rootNode // not mounted
      || !_this.portalNode // no portal
      || _invoke(_this, 'triggerNode.contains', e.target) // event happened in trigger (delegate to trigger handlers)
      || _invoke(_this, 'portalNode.contains', e.target) // event happened in the portal
      ) return; // ignore the click

the e.target, which should be the Modal's trigger DOM node, is never the DOM node that is rendered in the screen at that time. It's something with the exact same structure as the one displayed on screen but it's not actually in the DOM.

Inside my own handleTriggerClick though, the e.target is the correct one, i.e. the one rendered on screen at that time

As a result, the rest of the code is executed

      var didClickInRootNode = _this.rootNode.contains(e.target);

      if (closeOnDocumentClick && !didClickInRootNode || closeOnRootNodeClick && didClickInRootNode) {

        _this.close(e);
      }

and the Popup closes.

When I click on any other entry, which is not the enhancedTrigger, inside the Popup list, the Popup, does not close, as expected.

Now, if in my Modal I simply render the trigger that was passed as prop and not the enhancedTrigger that I create, the e.target in the Portal.js, lines 41-46, will be the correct one and the Popup will not close.

This does not make sense to me, since the element created with React.cloneElement() should have all of its properties referentially the same as the original trigger element's (except for the overriden onClick), and also point to the same DOM instance.

Am I missing something here with React.cloneElement() or what? Can somebody explain to me what goes wrong there with the e.target?

Thanks for taking the time to read all this

@layershifter
Copy link
Member

layershifter commented Oct 14, 2017

@mkarajohn There are too many text and too less code 😕 Please share to us some real code on https://codesandbox.io/ and describe expected and actual behaviour.

@mkarajohn
Copy link
Contributor Author

Yeah, sorry, will do. It was like 5am when I wrote that 😅

@mkarajohn
Copy link
Contributor Author

mkarajohn commented Oct 16, 2017

Ok, after further looking into this issue, it is not a Semantic UI issue, but rather something with React.cloneElement, which I am not taking into account. Feel free to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants