3
\$\begingroup\$

Learning React, I have created a basic App Component and Modal Component, the App Component renders 2 actions - hold and cancel, and a Modal based on the state modalVisible. I've kept the Modal Component light opting to pass the events back to the App Component (not sure if this is recommended), my reasoning is that this component could then be used by multiple actions. Would be great to get your thoughts in order to learn and improve!

/**
 * App Component
 * Holds actions, controls modal display using the state modalVisible.
 */
var AppComponent = React.createClass({
    getInitialState() {
        return {
            modalVisible: false
        }
    },

    _onModalYesClicked() {
        console.log('onYesClicked::contact the server');

        this.toggleModal();
    },

    _onModalNoClicked() {
        console.log('onNoClicked:contact the server');

        this.toggleModal();
    },

    toggleModal({title = null, description = null, cancelLabel = 'No', submitLabel = 'Yes'} = {}) {
        this.setState({
            modalVisible: !this.state.modalVisible,
            title: title,
            description: description,
            cancelLabel: cancelLabel,
            submitLabel: submitLabel
        });
    },

    onCancelClicked() {
        console.log('onCancelClicked');

        this.toggleModal({
            title: 'Cancel',
            description: 'Would you like to commit to cancelling?'
        });
    },

    onHoldClicked() {
        console.log('onHoldClicked');

        this.toggleModal({
            title: 'Hold',
            description: 'Are you sure you would like to hold the order?'
        });
    },

    render() {
        var modal = undefined;

        if(this.state.modalVisible) {
            modal = <ModalComponent 
                title={this.state.title}
                description={this.state.description}
                onModalYesClicked={this._onModalYesClicked} 
                onModalNoClicked={this._onModalNoClicked}  />
        }

        return (
            <div>
                <button className="btn btn--negative" onClick={this.onCancelClicked}>Cancel</button>
                <button className="btn" onClick={this.onHoldClicked}>Hold</button>

                {modal}
            </div>
        )
    }
});

/**
 * Modal Component
 * Uses props to tell parent component the action to take on action click.
 */
var ModalComponent = React.createClass({
    render() {
        return (
            <div className="modal modal--is-open">
              <div className="modal__inner">
                <div className="modal__contents" role="dialog" aria-hidden="false" aria-labelledby="modal__title" aria-describedby="modal__description">
                  <button className="modal__close" aria-label="close" onClick={this.props.onModalNoClicked}><i className="fa fa-close"></i></button>
                  <h1 className="modal__title">{this.props.title}</h1>
                  <p className="modal__description">{this.props.description}</p>
                  <button type="button" className="btn" onClick={this.props.onModalYesClicked}>{this.props.submitLabel ? this.props.submitLabel : 'Yes'}</button>&nbsp;
                  <button type="button" className="btn btn--secondary" onClick={this.props.onModalNoClicked}>{this.props.cancelLabel ? this.props.cancelLabel : 'No'}</button>
                </div>
              </div>
            </div>
        )
    }
});

/**
 *  Kicks the application off.
 */
ReactDOM.render(<AppComponent />, document.body.querySelector('.js-app'));

JSFiddle: http://jsfiddle.net/kyllle/xsrge3px/

\$\endgroup\$
2
\$\begingroup\$

I've kept the Modal Component light opting to pass the events back to the App Component

I believe this is the correct way to make components generic.

&nbsp;

OMG! Use margins instead. :P

I also notice you use the BEM naming scheme to specifically target styles to the component and not wreak havoc globally. To me, this is a big plus.

I notice that you either add or remove the modal depending on modalVisible. Since you're adding and removing the element from the DOM, the modal will pop in and out of existence and you have no room for transition effects. I think you should just leave the modal on the DOM rendered, but initially hidden. Then use modalVisible to toggle the presence of the modal--is-open modifier.

I also would discourage peppering your code with console function calls. It's debugging code, not for production and leaving it there is bad practice. Use breakpoints in the browser's debugger to inspect code.

One trick to still do console.log without ever putting them in your code is to use a conditional breakpoint but with the console.log call as the expression. When the breakpoint is hit, it is evaluated, console.log gets called. But since console.log returns undefined, which is falsy, the breakpoint won't pause.

The Right Way™ to do React apps is for your components to not to hold state... at all. State should be in stores. In your case, you just made your top-level component the holder of state, which is fine for small apps. Should it grow, consider using stores. I heard Redux is the simplest of them all.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.