dooboolab-community/hackatalk

Make ProfileModal simple by following structure of Ant-Design way

qkreltms opened this issue · 6 comments

Specify project
Client

I think the ProfileModal that we've using is bit hard to use and understand.
Because of it uses ContextAPI, when you use modal you must wrap your component with profileModalProvider

    <ProfileModalProvider>
      <RootNavigator />
    </ProfileModalProvider>

In addition, you should put your modal on root component MainStackNavigator.tsx and also inject it's ref to the store, by following:

  const { state } = useProfileContext();
  const modalEl = useRef(null);
  state.modal = modalEl;

  <ProfileModal
        testID="modal"
        ref={state.modal}
        //....

As you can see the ProfileModal and it's store is also separated.

I suggest the Ant-Design way to make it simple:

https://github.com/ant-design/ant-design/blob/ea19589619f4de24a4a1a3d38bd4330bb6b00223/components/modal/confirm.tsx#L33-L103

export default function confirm(config: ModalFuncProps) {
  const div = document.createElement('div');
  document.body.appendChild(div);

   //...

    function render({ okText, cancelText, prefixCls, ...props }: any) {
      //...
      ReactDOM.render(
        <ConfirmDialog
          {...props}
          prefixCls={prefixCls || `${getRootPrefixCls()}-modal`}
          rootPrefixCls={getRootPrefixCls()}
          okText={okText || (props.okCancel ? runtimeLocale.okText : runtimeLocale.justOkText)}
          cancelText={cancelText || runtimeLocale.cancelText}
        />,
        div,
      );
  }
   
  render(currentConfig);
}

You can simply call modal by doing so: confirm({ ... })

Looks like a great idea. Could you kindly give a proposal to this approach with PR?

It seems like we're using react-native-modalbox. Is there any reason why we don't use the default Modal from React-Native?
https://reactnative.dev/docs/modal

@0916dhkim It seems react-native-modalbox can't called by function call?

It seems react-native-modalbox can't called by function call?

@qkreltms No it can't. What do you think of changing ProfileModalProvider to return this context:

interface State {
  user: User;
  deleteMode: boolean;
  onDeleteFriend?: () => void;
  onAddFriend?: () => void;
}

interface Context {
  state: State;
  isVisible: boolean;
  showModal: (next: State) => void;
  hideModal: () => void;
}

Instead of the current implementation:

interface ShowModalParams {
  user: User;
  deleteMode: boolean;
  onDeleteFriend?: () => void;
  onAddFriend?: () => void;
}

export interface State {
  user: User;
  deleteMode: boolean;
  modal?: React.MutableRefObject<ProfileModalRef | null>;
}

interface Context {
  state: State;
  showModal: (showModalParams: ShowModalParams) => void;
}

By taking the first approach, consumers of ProfileModalProvider do not need to worry about checking ref every time.
Also, this allows us to remove strong bonding between ProfileModal & ProfileModalProvider.
We can simply implement ProfileModal with RN Modal that consumes ProfileModalProvider.
(ProfileModalProvider not holding ref of ProfileModal anymore).

I've made some changes in #275
Closing this issue.
@qkreltms Please re-open the issue if you think there is a room to improve.

@0916dhkim Sorry for long waiting. We still need ProfileModalProvider to use just one ProfileModal, but I'm satisfied that it becomes more easy to use.
By the way, I've done making an example. feel free to look around it.