Refactor/extract expose dialog utility components

[shreedharbhat98]

Overview

Extracted the renderHeader , renderChildren, renderFooter JSX functions and exposed them as standalone components.

Screenshots (if applicable)

Documentation

• Updated Typescript types and/or component PropTypes
• Added / modified component docs
• Added / modified Storybook stories

[shreedharbhat98]

Hey @brandongregoryscott Would you mind reviewing this PR?
I want to make sure about the backward compatibility, so feel free to give suggestions. Also Can you please tell me how to generate the related docs, please?

[netlify]

✅ Deploy Preview for evergreen-storybook ready!

Name	Link
🔨 Latest commit	6b7c814
🔍 Latest deploy log	https://app.netlify.com/sites/evergreen-storybook/deploys/628f6f5839bd3800084e87d3
😎 Deploy Preview	https://deploy-preview-1470--evergreen-storybook.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[brandongregoryscott]

Hey @shreedharbhat98! Thanks for the contribution. This one has been on my mind for a while now. I pulled it down and I'm playing around with it in Storybook, I think one thing that is crucially missing is the close function from the render props provided by Overlay. This is the 'behind-the-scenes' close handler that lets the Dialog close itself even if you don't pass any custom onCancel or onConfirm handlers.

I left some general cleanup comments for the structure of how the components are broken out, but I think I need to take a step back and figure out what the API for these utility components should look like. Ideally, I wanted them to be broken out in a way that you could pass them to the header/footer prop of the Dialog and get the outer styling for free, something along the lines of:

<Dialog
  header={
    <Dialog.Header>
      <Heading>Hello world</Heading>
      <Link>View documentation</Link>
    </Dialog.Header>
  }
>
  {/* ... */}
</Dialog>

However, the close function that we get from the Overlay's render props complicates this. It feels like we need some way to pass through this function from the Dialog to the DialogHeader and DialogFooter components, but you wouldn't have it available in the above example ☝️ unless you used a function to pass it along, i.e:

<Dialog
  header={({ close }) =>
    <Dialog.Header>
      <Heading>Hello world</Heading>
      <Link>View documentation</Link>
      <IconButton icon={CrossIcon} onClick={close}/>
    </Dialog.Header>
  }
>
  {/* ... */}
</Dialog>

(and that might be okay, too. It's kinda how the header prop works right now - you can pass arbitrary JSX, but you won't get the close functionality for free without using the function variant.)

Wanted to give you some initial feedback, but I'll try to report back here or on the original issue and tag you when I have more specific feedback on what the props/implementation should look like. Hope this helps for now!

[brandongregoryscott]

Hey @shreedharbhat98 - sorry for the delay. I pushed out a branch on my fork that has the API design I was thinking of:
https://github.com/segmentio/evergreen/compare/master...brandongregoryscott:extract-dialog-components?expand=1

The two major changes are the addition of the optional close prop to the DialogHeader and DialogFooter components, and the way those components are rendered from inside the Dialog.

1. The close prop is optional because it might not be provided if the user doesn't need to close the Dialog from that section (basically the same behavior as now). It needs to be passed in separately from the onCancel/onConfirm handlers to be "magically closed" when those functions aren't provided, and it is passed along to give the consumer more control over custom confirming or cancelling logic.

2. The Dialog component still uses 'render' functions for each section (header, body, and footer) to allow complete overrides of the header and footer when those props are provided. With the way you are currently rendering each of those, there's no way to "opt out" of the wrapping DialogHeader or DialogFooter components like we currently have - you'd always be confined within the children of those components.

Finally, for a better developer experience, I added a default close function which logs a warning in development when this function is called, because it probably means they are trying to call it from onCancel or onConfirm without actually forwarding it from the Dialog render props.

Hope this helps! Let me know if I can add any additional context or clarification.

[shreedharbhat98]

https://github.com/segmentio/evergreen/compare/master...brandongregoryscott:extract-dialog-components?expand=1

Thanks for the suggestions @brandongregoryscott . I will try to implement this. Thanks a bunch

[shreedharbhat98]

Hey @shreedharbhat98 - sorry for the delay. I pushed out a branch on my fork that has the API design I was thinking of: https://github.com/segmentio/evergreen/compare/master...brandongregoryscott:extract-dialog-components?expand=1

The two major changes are the addition of the optional close prop to the DialogHeader and DialogFooter components, and the way those components are rendered from inside the Dialog.

1. The close prop is optional because it might not be provided if the user doesn't need to close the Dialog from that section (basically the same behavior as now). It needs to be passed in separately from the onCancel/onConfirm handlers to be "magically closed" when those functions aren't provided, and it is passed along to give the consumer more control over custom confirming or cancelling logic.
2. The Dialog component still uses 'render' functions for each section (header, body, and footer) to allow complete overrides of the header and footer when those props are provided. With the way you are currently rendering each of those, there's no way to "opt out" of the wrapping DialogHeader or DialogFooter components like we currently have - you'd always be confined within the children of those components.

Finally, for a better developer experience, I added a default close function which logs a warning in development when this function is called, because it probably means they are trying to call it from onCancel or onConfirm without actually forwarding it from the Dialog render props.

Hope this helps! Let me know if I can add any additional context or clarification.

Hey @brandongregoryscott As we use the render functions, do we need to update the stories here?

[brandongregoryscott]

Hey @brandongregoryscott As we use the render functions, do we need to update the stories here?

Since these components are mostly used for composition and aren't really meant to be used on their own, I would say there's little value in building out new stories for them.