Refactor/extract expose dialog utility components #1470

shreedharbhat98 · 2022-05-14T14:18:05Z

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 · 2022-05-14T14:18:58Z

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 · 2022-05-14T14:21:14Z

✅ 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!

src/dialog/src/Dialog.js

src/dialog-body/src/DialogBody.js

src/dialog-body/index.js

src/dialog-body/src/DialogBody.js

src/dialog-body/stories/index.stories.js

src/dialog-footer/stories/index.stories.js

src/dialog/src/Dialog.js

…tility-components

brandongregoryscott · 2022-05-23T00:45:34Z

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.

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.
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 · 2022-05-23T15:39:38Z

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

…tility-components

shreedharbhat98 · 2022-05-26T13:38:07Z

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.

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.

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 · 2022-05-26T17:26:54Z

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.

shreedharbhat98 added 5 commits May 14, 2022 08:29

Bootstrapped dialog components

2b8d55d

refactored and moved DialogBody

7ca4661

refactored and moved DialogFooter

4d6a1b2

refactored and moved DialogHeader

a15cf48

composit usage of DialogHeader, DialogBody and DialogFooter in Dialog

63935ef

brandongregoryscott linked an issue May 16, 2022 that may be closed by this pull request

Extract & expose Dialog utility components #1466

Open

brandongregoryscott reviewed May 16, 2022

View reviewed changes

Merge branch 'segmentio:master' into refactor/Extract-expose-Dialog-u…

2c5aa3b

…tility-components

Merge branch 'segmentio:master' into refactor/Extract-expose-Dialog-u…

6b7c814

…tility-components

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/extract expose dialog utility components #1470

Refactor/extract expose dialog utility components #1470

shreedharbhat98 commented May 14, 2022 •

edited

shreedharbhat98 commented May 14, 2022 •

edited

netlify bot commented May 14, 2022 •

edited

brandongregoryscott left a comment

brandongregoryscott commented May 23, 2022

shreedharbhat98 commented May 23, 2022

shreedharbhat98 commented May 26, 2022 •

edited

brandongregoryscott commented May 26, 2022

Refactor/extract expose dialog utility components #1470

Are you sure you want to change the base?

Refactor/extract expose dialog utility components #1470

Conversation

shreedharbhat98 commented May 14, 2022 • edited

shreedharbhat98 commented May 14, 2022 • edited

netlify bot commented May 14, 2022 • edited

✅ Deploy Preview for evergreen-storybook ready!

brandongregoryscott left a comment

Choose a reason for hiding this comment

brandongregoryscott commented May 23, 2022

shreedharbhat98 commented May 23, 2022

shreedharbhat98 commented May 26, 2022 • edited

brandongregoryscott commented May 26, 2022

shreedharbhat98 commented May 14, 2022 •

edited

shreedharbhat98 commented May 14, 2022 •

edited

netlify bot commented May 14, 2022 •

edited

shreedharbhat98 commented May 26, 2022 •

edited