minutemailer / react-popup

React popup component
http://minutemailer.github.io/react-popup/
MIT License
208 stars 71 forks source link

Component state is not persisted #31

Closed z-vr closed 6 years ago

z-vr commented 7 years ago

The component state is not persisted when opening a new popup, then coming back to the old one. Checkout the demo - open a new popup, then use a button inside the popup to create a new one.

popup
constructor 1
component 1 will mount
popup
component 1 will unmount
constructor2
component 2 will mount

Now if you close the second popup, you will see:

component 2 will unmount
constructor 1
component 1 will mount

what we don't really want is constructor 1 happening again, because we want to keep the state of the first component.

I think when you do

    onClose() {
        this.setState(initialState); // set content to null
    }

This destroys

                    <div className={this.className('box__body')}>
                        {this.state.content}
                    </div>

and also probably when switching between contents, react is not able to restore them afterwards. A solution would be to keep all contents in a popup dom tree hidden, and only show the relevant one? Can't think of anything else. As a workaround, I have to pass state in a mutable object when opening popup.

Here's the source code

Have you got any thoughts on this?

tbleckert commented 7 years ago

Ah, hm. Didn't think about that scenario. That complicates stuff for sure. I need to give this some thought. I think some sort of navigation wrapper would be the best in these sorts of scenarios. Instead of multiple popups you have one popup that can hold multiple views.

class ContentNavigation extends React.Component {

    constructor(props) {
        super(props);

        this.state = {
            views: this.props.views,
            currentView: 0
        };
    }

    render() {
        return this.state.views[this.state.currentView];
    }

}

Then you would pass this as the content to the popup and either use buttons inside this component or set it up so your popup buttons can navigate.

Seems more natural to have it like this. If for example you're displaying a form with multiple steps inside the popup it feels more "correct" (in lack of a better word) to have a form component that manages the steps.

What do you think? This could be made into a plugin and used like this:

Popup.plugins().multipleViews('The title', [/* Array of components/views */], {/* object of labels for buttons: next, back, cancel, complete */});
z-vr commented 7 years ago

@tbleckert yeah it definitely makes things trickier. I like the views idea, but in my particular case I don't have step-by-step form completion, I open popups from within a form popup, e.g.,

LOCATION

Location: [_______]
Candidate: [_______]
   (add candidate)
Office Address: [_______]
   (add address)

The views are dynamic here, and it would be better to have an internal mechanism dealing with it, when pushed / popped from the queue.

It is true that a form can have a component that manages steps, but I don't think it would be 100% relevant to the popup function, since it's very case-specific.

tbleckert commented 6 years ago

I've thought about this but can't really come up with a good way to handle this. And I'm not sure if this is something that should be handled inside the component.

The best solution, imo, is to manage the state in your application. For example, you can have a component that saves all changes through Redux and always re-apply it when mounted.

What do you think about that? I'm closing but feel free to comment and/or reopen.