Feat/revamp modal

[pKorsholm]

What

• create new molecule Modal using radix-ui
• Allow switching between large and small modals with a boolean

How

• adapt the Dialog component from radix-ui
• clone child elements as the modal is created, setting the isLargeModal prop of each child
  • I would like some feedback on the design choice here

Adding props through the DOM tree
When creating this component i thought it would be nice to be able to define if a modal was large or small with a single boolean value. Since different parts of the modal depends on this, specifically the padding and border of the footer, I looked at how to pass props to the {children} element at runtime.
I found two different options for how to achieve this:

1. Clone the child elements and pass isLargeModal as a prop throughout the DOM tree
2. Create a local context which would be replicated for each modal

I chose option nr. 1 since it seemed the least over-engineered and from what I could read using React.cloneElement does not mean taking a performance hit in terms of the render, especially when working with such small components as modals. Using contexts is something we can always do instead at a later time because of the relative small size of the component, though the usecase seems to be for bigger elements of an application rather than an invite modal.

I have implemented this practically as a helper method in the index.js file of the modal molecule. The helper method uses the React.cloneElement method to add the prop to either a single element or array of elements:

const addProp = (children, prop) => {
  return !Array.isArray(children) ? React.cloneElement(children, prop) : 
    children.map(child => React.cloneElement(child, prop))
}

This method is then used in each of the Modal components and subcomponents to add the isLargeModal prop to each of the children as such:

const Modal = ({children, onClick, isLargeModal, ...rest}) => {
  const largeModal = isLargeModal
  return (
    <Dialog.Root open={true} onOpenChange={onClick}>
      <Dialog.Portal>
        <Overlay>
          <Content>
            {addProp(children, largeModal)}
          </Content> 
        </Overlay>
      </Dialog.Portal>
    </Dialog.Root>)
}

Example
Below is an example of how to use the modal, each part of the modal is optional, and the top-level Modal component has a prop isLargeModal to indicate if the modal has width 750 or if it's smaller.

furthermore when isLargeModal is set it will make the footer wider and add a hairline border top to the footer.

return (
    <Modal isLargeModal={true} onClick={handleClose}>
      <Modal.Body >
        <Modal.Header handleClose={handleClose} justifyContent="space-between" alignItems="center">
          <span className="text-large font-semibold">Invite Users</span>
        </Modal.Header>
        <Modal.Content flexDirection="column">
            <InputField
              placeholder="lebron@james.com"
              value={email}
              onChange={e => setEmail(e.target.value)}
            />
        </Modal.Content>
        <Modal.Footer>
          <div className="flex w-full justify-end">
            <Button mr={2} variant="primary" onClick={handleClose}>
              Cancel
            </Button>
            <Button loading={isLoading} onClick={handleSubmit} variant="cta">
              Invite
            </Button>
          </div>
        </Modal.Footer>
      </Modal.Body>
    </Modal>
  )

[olivermrbl]

LGTM with comment.

Really like the proposed solution. It's very clean and simple to use. Had a tiny comment on the way we render children. Let me know, what you think :)

Would also argue, that we should add PropTypes for the modal until we have Typescript in place. This will allow other devs. to very easily use and configure the modal without having to dig into the implementation specifics. What do you think?

[zakariaelas]

Great work Philip! Loved the cloning approach and I think it's cleaner/simpler than using contexts for that matter 👍 Left a few comments here and there which are all up for discussion!

Btw, I noticed that you use rest parameters for the components, which imo is a good thing when it comes to extending or building around a library. However, I'm not sure if they're passed along the children components.

Nevertheless I think it's not a very straightforward "design problem" especially for components like Modal which wrap around Dialog.Root, Dialog.Portal, Overlay, and Content.

What I would suggest is to pass Modal's rest params to Dialog.Root, then if we come across a use case where we need to pass props to Dialog.Portal or any other component, we can extend the prop types to something like portalProps and/or overlayProps etc.

Feel free to skip this comment btw because at this stage, I think it's not extremely important, and we can always think about this later down the road. Just thought that the idea of passing down some props to radix primitives might be worth keeping in mind.

[zakariaelas]

I'm thinking we might want to take a triggerElement as prop and render it as a child of <Dialog.Trigger>. My understanding is that Dialog.Trigger encapsulates some important HTML attributes for accessibility to signal that a button opens an interactive element (a modal in this case).

I'm thinking something like:

const Modal = ({ triggerElement, ...otherProps }) => {
  return (
    <Dialog.Root>
      {triggerElement && <Dialog.Trigger>{triggerElement}</Dialog.Trigger>}
      ...
    </Dialog.Root>
  )
}

[zakariaelas]

I think it would cool not to hard code the open state and instead take it as props. Same for onOpenChange because the callback is called whenever the open state changes (so from open -> closed and from closed -> open). So the onClick might not be 100% accurate. Let me know what you think

[zakariaelas]

would be cool to have an additional Modal.Close = Dialog.Close here so that we have more than one button to close the button (close icon on the top + cancel button).

[zakariaelas]

What do you think about wrapping the span in a Dialog.Close? The Dialog.Close will take care of closing the modal.