Refactor Popups

[liumcse]

The current way of using Popups makes it incredibly difficult to add new Popups, therefore I'm refactoring the code to make it easier to call Popups.

Close #68

[endiliey]

Hey @lyming90,

I haven't really seen through the code, but from what I've noticed is that this PR try to wrap the popup/modal as HOC.

What do you think of triggering modal/popup just by dispatching a redux action ? This is quite a common practice and pretty scalable, recommended by Dan Abramov. You can easily pass props through redux action as well

[liumcse]

@endiliey Wow... Surprisingly that's what is being used right now... However, I find calling Popups using redux brings in lots of code - you have to connect to redux, and map the states and dispatch and so on. And if you wanna add new popups you have to modify the action and reducer as well... which is why I find it tedious and I'm trying to make it easier so I've been trying to separate it from the redux logic. I didn't know using Redux to control Popup is a common practice though, thanks for letting me know, guess I have to think again

[endiliey]

Actually, the concept is similar, but the implementation is different.

Adding new popups should not need you to modify the reducer and action at all.

Try looking at https://stackoverflow.com/questions/35623656/how-can-i-display-a-modal-dialog-in-redux-that-performs-asynchronous-actions again.

You can see my implementation for a project that I recently did https://github.com/endiliey/rengorum/blob/master/frontend/src/actions/modal.js Example snippets:

export const showModal = (modalType, modalProps) => ({
  type: SHOW_MODAL,
  modalType,
  modalProps,
});

export const hideModal = () => ({
  type: HIDE_MODAL,
});

https://github.com/endiliey/rengorum/blob/master/frontend/src/reducers/modal.js Example snippets:

const initialState = {
  modalType: null,
  modalProps: {},
};

const modal = (state = initialState, action) => {
  switch (action.type) {
    case SHOW_MODAL:
      return {
        modalType: action.modalType,
        modalProps: action.modalProps,
      };
    case HIDE_MODAL:
      return {
        modalType: null,
        modalProps: {},
      };
    default:
      return state;
  }
};

export default modal

Adding new modal is in fact just adding new switch component to Root Modal Component https://github.com/endiliey/rengorum/blob/master/frontend/src/containers/modal/index.js Example snippets:

import React from 'react';
import {connect} from 'react-redux';
import RegisterModal from './register';
import LoginModal from './login';
import EditProfileModal from './editprofile';

const ModalContainer = props => {
  switch (props.modalType) {
    case 'REGISTER':
      return <RegisterModal />;
    case 'LOGIN':
      return <LoginModal />;
    case 'EDIT_PROFILE':
      return <EditProfileModal />;
    default:
      return null;
  }
};

const mapStateToProps = state => ({
  modalType: state.modal.modalType,
  modalProps: state.modal.modalProps, // for future use if need to pass props
});

export default connect(mapStateToProps)(ModalContainer);

Another thing is that using local state defeats the purpose of having Redux, which is a predictable state container. And of course, the downside of Redux is that there is a lot of boilerplate.

[liumcse]

@endiliey That looks really good, I'll have to think again on the implementation then!

[endiliey]

Sure, no worries. You don't really have to do that, it should be up to you as you are the code owners.

But I do suggest to use common best practices, and if you plan to open source it one day, might as well start refactoring from today.

Another thing is that I'd recommend common folder structures to make it easy for people to manage stuff.

Also mentioned in https://redux.js.org/faq/codestructure#code-structure

www
  src/
    actions/ # redux actions
    api/ # all api calls
    components/ # dumb components only receive props. No side effects/ connect to Redux. Easily testable
    containers/ # all view/screen that is connected to Redux
    reducers/ # your reducers
    assets/ # this is all your static assets

In addition, since we alias on webpack side. We can then import stuff by doing import blabla from '@reducers/home' for example. import Avatar from '@components/avatar'. Looks declarative !

[liumcse]

@endiliey Yeah using the best practice is the point, exactly what I want to achieve as well.

About the code structure, the current way is to create a folder for each page and put subsequent components inside that folder. I think it's more feature-oriented, not sure which way is better though (but certainly we need to create the api folder as the current api calls are quite messy).

[endiliey]

About the code structure, the current way is to create a folder for each page and put subsequent components inside that folder.

Yup, feature oriented is good, but its quite unclear to see whether that is actually a page, an api, an utils function, a React component that is connected to redux, an utils or just dumb React component. What if we have 100 pages (a.k.a views)?

How about just grouping all the pages into one folder named containers for better higher level of overview

P.S: Feel free to decide. Just adding suggestion here, 😃