1

I have a Modal component which is a popup window. I need to set different components to the bodyContainer.

<Modal
    show={this.props.first.showModal}
    size={this.props.first.sizeModal}
    bodyContainer={this.props.first.modalBodyContainer}
    /* bodyContainer={<Mail {...this.props}/>} */
    onClose={this.props.firstActions.onCloseModalHome}
/>

As I understand the redux philosophy I can do it like this

// Search Patient actions
export const onSearchPatient = (ur, token) => dispatch => (
  callToApiWithToken(`patient/by/ur/${ur}`, token)
    .then(response =>
      ((response.data === null) ? dispatch(onSearchPatientNotFound(ur)) : dispatch(onSearchPatientFound(response.data))),
    )
    .catch(error => ({ type: psActions.ON_SUBMIT_ERROR }))
);

export const onSearchPatientFound = createAction(psActions.ON_SEARCH_PATIENT_FOUND);// data
export const onSearchPatientNotFound = createAction(psActions.ON_SEARCH_PATIENT_NOT_FOUND);

//Reducer 
case psActions.ON_SEARCH_PATIENT_FOUND:
  return {
    ...state,
    showModal: true,
    modalBodyContainer: <PatientDetails {...action.payload} />,
  };

case psActions.ON_SEARCH_PATIENT_NOT_FOUND:
  return {
    ...state,
    error: true,
    errorMsg: <div>The Patient <strong>{action.payload}</strong> is not in the system</div>,
  };

But my colleague arguing that this is a bad practice. Specifically I'm talking about

modalBodyContainer: <PatientDetails {...action.payload} />

It is possible to relocate render logic to the Modal but in this case I need to create a switch for all possible components.

What is the right way to do this?

Edited I have two ideas how to do this. What is the best to use?

//Action
export const setModalBody = createAction(psActions.SET_MODAL_BODY);//component

//Reducer 
case psActions.SET_MODAL_BODY:
  return {
    ...state,
    showModal: true,
    modalBodyContainer: action.payload />,
  };
//Usage
onClick={() => searchPatient.length > 0 && onSearch(searchPatient, token).then(patientFound && setModalBody(<Patient {...props} />)

OR

const Modal = ({...}) => (
{{
//Something like this
//switch based on prop.bodyComponentType
//in this case I need to import all possible components to the Modal
    sectionA: (
      <SectionAComponent />
    ),
    sectionB: (
      <SectionBComponent />
    ),
    sectionC: (
      <SectionCComponent />
    )
  }[section]}
)

Edited 2 It is not about redux actions, it's about a component rendering inside hoc related to actions.

victor zadorozhnyy
  • 889
  • 1
  • 12
  • 25
  • Possible duplicate of [How can I display a modal dialog in Redux that performs asynchronous actions?](https://stackoverflow.com/questions/35623656/how-can-i-display-a-modal-dialog-in-redux-that-performs-asynchronous-actions) – lux Sep 12 '17 at 18:48
  • @lux i don't think so. it is not about actions it's about components rendering and how to implement it based on redux philosophy. please if you can suggest the right way to do it – victor zadorozhnyy Sep 12 '17 at 18:56
  • Don't make the body dynamic, just issue an action that renders a specific modal. No need to get clever and swap out the bodies since you already have a generic Modal component. – lux Sep 12 '17 at 18:59
  • @lux so the setModalBody action is a good idea, right? – victor zadorozhnyy Sep 12 '17 at 19:10
  • 1
    What about something like this: https://gist.github.com/mikechabot/975eab040ca70e36bf84579453d530e5 Where your redux action has a `modalType` that refers to a fully built modal, and you simply and only pass in `props` that are required by that Modal, not the entire DOM body of it. – lux Sep 12 '17 at 19:13
  • @lux good idea! so I can only require exact type of a body component and do not import all types. please post it below – victor zadorozhnyy Sep 12 '17 at 19:38

1 Answers1

3

Your coworker is right.

The philosophy of Redux/React is largely around separation of concerns and making things loosely coupled but highly cohesive. For this situation, that means that your action should not know anything about the component it will cause to render (loosely coupled), but will be a complete definition of the result of your api call (highly cohesive).

I would have both of your reducer functions modify the same attribute of the redux state. Afterall, they represent the same piece of data, just with different values. One represents a success, and the other a failure. So I would have them both modify patientFoundStatus and provide it a value (perhaps using http codes, or a success/failure constant defined elsewhere). Then use this value to determine the output of your modal.

So why is this better?

Well for a couple of reasons. The most important is to consider how your code would need to be modified if, later on, this action needed to produce a different (additional) result in the view. Let's say that it is now used to pre-populate a form with the patient's information. How would your code need to change?

Given the code you have above, it would need to be entirely rewritten or duplicated. The modal body components would be completely irrelevant to this new feature, and the "success/failure" information (and patient information) needed would need to be added.

But consider if these actions only returned the data relevant to the view. Well now, what you do with that data is up to you. The reducers and actions could remain entirely unchanged, and you could merely use that pre-existing slice of state to impact a new section of your app.

That's one of the biggest advantages to react/redux when used correctly. Over time, what you end up creating is a flexible toolbox that makes adding new features trivial, because all the plumbing is well in place and wholly distinct from how it is utilized. Low coupling. High cohesion.

Jake Haller-Roby
  • 6,335
  • 1
  • 18
  • 31
  • please have a look at my update. what is the right/best way to do this – victor zadorozhnyy Sep 12 '17 at 17:57
  • I probably wouldn't do either. There's no reason for a single component to render every modal if those modals serve different purposes. What you really want is to create a "higher order component" that provides the modal functionality to different "body" components, and keep those body components de-centralized. https://facebook.github.io/react/docs/higher-order-components.html – Jake Haller-Roby Sep 12 '17 at 19:02
  • I have different hoc components for all records type. from home page need to show existing records and onclick show populated hoc in modal. Is it not overkilling to wrap hoc to another hoc just to show a popup? – victor zadorozhnyy Sep 12 '17 at 19:24