segmentio / evergreen

🌲 Evergreen React UI Framework by Segment
https://evergreen.segment.com
MIT License
12.39k stars 832 forks source link

Refactor/extract expose dialog utility components #1470

Open shreedharbhat98 opened 2 years ago

shreedharbhat98 commented 2 years ago

Overview

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

Screenshots (if applicable)

Documentation

shreedharbhat98 commented 2 years ago

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[bot] commented 2 years ago

Deploy Preview for evergreen-storybook ready!

Name Link
Latest commit 6b7c814fb75e9151a41133daf734b5237065d45e
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...

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.