Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dialog] Add optional Title and Description parts #741

Merged
merged 7 commits into from Jun 23, 2021

Conversation

benoitgrelard
Copy link
Collaborator

Closes #729

This PR:

  • brings Title and Description from AlertDialog into Dialog itself
  • improves upon existing dev warnings with new requirements
    • description is optional for Dialog but required for AlertDialog so that's reflected in the warnings
    • "localizes" the messages based on which primitive it is
    • links to docs

packages/react/alert-dialog/src/AlertDialog.tsx Outdated Show resolved Hide resolved
packages/react/dialog/src/Dialog.tsx Outdated Show resolved Hide resolved
packages/react/dialog/src/Dialog.tsx Outdated Show resolved Hide resolved
packages/react/dialog/src/Dialog.tsx Outdated Show resolved Hide resolved
packages/react/dialog/src/Dialog.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@jjenzz jjenzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial feedback to discuss. I'll finish reviewing when we've decided the outcome of this in case things change.

packages/react/alert-dialog/src/AlertDialog.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@jjenzz jjenzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looking good. Just one little nit that I'll leave up to you 😛

Close,
//
LabelWarningContext,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we expose just LabelWarningProvider instead please? It looks like we only need the provider externally and we could also simplify consumption then:

const LabelWarningProvider: React.FC<LabelWarningContextValue> = props => {
  return <LabelWarningContext.Provider value={props} />;
}
<Dialog.LabelWarningProvider contentName="" titleName="" docsSlug="">
</Dialog.LabelWarningProvider>

vs

<Dialog.LabelWarningContext.Provider value={React.useMemo(() => ({ contentName: '', titleName: '', docsSlug: '' ), [])}>
</Dialog.LabelWarningContext.Provider>

I'm not too bothered about doing the memoization internally though, it was more Dialog.LabelWarningProvider vs Dialog.LabelWarningContext.Provider that was bothering me so the following would suffice if you prefer 😅

const LabelWarningProvider = LabelWarningContext.Provider;
export { LabelWarningProvider };

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with the latter, because the former was missing the fact it should render children which brings us back in the same situation as createContext.

@benoitgrelard benoitgrelard merged commit d05d401 into main Jun 23, 2021
@benoitgrelard benoitgrelard deleted the dialog-title-description branch June 23, 2021 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dialog] Consider adding optional Title and Description parts like in AlertDialog
2 participants