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

Portal reopens if parent component sets its state immediately after .closePortal() #91

Closed
wants to merge 3 commits into from

Conversation

bevesce
Copy link

@bevesce bevesce commented Aug 10, 2016

Minimal example:

class NotClosingPortalContainer extends React.Component {
    constructor() {
        super(...arguments)
        this.state = {counter: 0}
        this.handleClick = () => {
            this.setState({counter: this.state.counter + 1})
        }
    }

    render() {
        return (
            <div>
                {this.state.counter}
                <Portal openByClickOn={<button>open</button>}>
                    <InsidePortal onClick={this.handleClick}>+1</InsidePortal>
                </Portal>
            </div>
        )
    }
}


class InsidePortal extends React.Component {
    constructor() {
        super(...arguments)
        this.handleClick = () => {
            this.props.closePortal()
            this.props.onClick()
        }
    }

    render() {
        return <button onClick={this.handleClick}>+1</button>
    }
}

Expected behaviour:

  • NotClosingPortalContainer displays counter and button to open portal
  • in the portal is a button that should increase counter and close the portal

What actually happens:

  • after clicking button inside portal it is closed and then immediately reopened

I think this happens:

  1. in Portal#closePortal this.setState({ active: false }) is called, but it works asynchronously
  2. NotClosingPortalContainer#handleClick in this.setState(...) is called
  3. NotClosingPortalContainer rerenders before Portal#state.active actual value is set to false
  4. Portal receives new props and opens portal (because Portal#state.active still equals true)
  5. Everything ends in a weird state

I fixed this by basically replacing Portal#state.active with Portal#active but it feels dirty. I'm pretty new to React so maybe there is a better solution?

Tests are passing.

@jdelStrother
Copy link

jdelStrother commented Aug 31, 2016

I believe I'm also running into this bug, but only in IE - not sure if React uses a slightly different rendering strategy there. @bevesce are you seeing it in all browsers?

[EDIT: scrap that, I do see it in all browsers. The IE behaviour I'm seeing is caused by something else]

@tajo
Copy link
Owner

tajo commented Dec 29, 2016

This is interesting, thanks! I can confirm the weird behaviour. I think you are right with the explanation. I don't see a problem with replacing Portal#state.active with Portal#active since we don't need to trigger Portal#render anyway. Not sure why I used state in the first place other than it felt more "react way". Or am I missing something?

If not, can you please rebase this PR so it's mergeable? Thanks.

@tajo tajo added this to the v4 milestone Dec 29, 2016
@tajo tajo added the bug label Dec 29, 2016
@bevesce
Copy link
Author

bevesce commented Jan 25, 2017

I've resolved conflicts

Copy link

@mattkrick mattkrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bevesce
Copy link
Author

bevesce commented Feb 3, 2017

Done

@dwick
Copy link

dwick commented Apr 20, 2017

this appears to be the same issue as #124, but #124 is a bit cleaner

@jtrang
Copy link

jtrang commented Apr 25, 2017

This solution works for me too! However, it solves a different issue than #124.

@tajo
Copy link
Owner

tajo commented Oct 1, 2017

Hey, please check the new major version (complete rewrite) of react-portal: #157

It's React v16 only since its uses the new official Portal API. There is the first beta released and I would like to get your feedback. I don't have bandwidth to maintain v3 which is very different and full of hacks.

If you feel your PR still applies, please rebase it against the master and re-open it. Sorry for the lack of response in past! Thanks!

@tajo tajo closed this Oct 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants