Skip to content

ConfirmationDialog - #1181

Merged
T-Hugs merged 20 commits into
mainfrom
confirm
Apr 23, 2021
Merged

ConfirmationDialog#1181
T-Hugs merged 20 commits into
mainfrom
confirm

Conversation

@T-Hugs

@T-HugsT-Hugs commented Apr 21, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

This PR adds a ConfirmationDialog component and a helper hook, useConfirm. A ConfirmationDialog is an opinionated dialog that is used specifically to confirm a user's choice. See the rendered documentation for more details.

Closes#1065

Screenshots

image

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@vercel

vercelBot commented Apr 21, 2021•
edited
Loading

Copy link
Copy Markdown

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/HmeyoR757LBjNvnZBxpwjujBC3KY
✅ Preview: https://primer-components-git-confirm-primer.vercel.app

@changeset-bot

changeset-botBot commented Apr 21, 2021•
edited
Loading

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: b472953

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

smockle
smockle reviewed Apr 22, 2021
View reviewed changes
Comment threaddocs/content/Dialog2.mdx Outdated
const confirm = useConfirm()
const buttonClick = React.useCallback(
e => {
// @todo The docs site live code examples don't work with async/await, so using the raw promise instead!

@smocklesmockleApr 22, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of curiosity: Do you know why the docs site live code examples don’t work with async/await? And is that tracked in an issue?

@T-HugsT-HugsApr 22, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

My guess is it's an old babel dependency or configuration. I don't believe it's tracked in an issue. I assume this would be a doctocat bug? I'm not exactly sure how the live code samples get compiled.

@colebemiscolebemisApr 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like there's an open issue for it in react-live (the library we use for live code examples) FormidableLabs/react-live#206

@T-HugsT-HugsApr 22, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated the sample to use async function() since apparently that works 🤷

VanAnderson
VanAnderson approved these changes Apr 22, 2021
View reviewed changes

@VanAndersonVanAnderson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a couple of small things I saw that I had some questions on, probably nothing blocking. Really great job on this!

Comment threaddocs/content/Dialog2.mdx Outdated
Comment threaddocs/content/Dialog2.mdx Outdated

| Prop name | Type | Default | Description |
| :----------------- | :-------------------------------------------------------------------- | :--------- | :---------------------------------------------------------------------------------------------------------------------------- |
| title | `string` | | Required. Sets the title of the dialog, which by default is also used as the `aria-labelledby` attribute. |

@VanAndersonVanAndersonApr 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

has the behavior of assigning the title to aria-labelledby been implemented? Can't see where that is happening in either Dialog or ConfirmationDialog

@T-HugsT-HugsApr 23, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It happens here: https://github.com/primer/components/blob/main/src/Dialog/Dialog.tsx#L286

Were you expecting something else?

@VanAndersonVanAndersonApr 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

whoops, must have missed that!

dgreif
dgreif suggested changes Apr 22, 2021
View reviewed changes

@dgreifdgreif left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Really cool to see these components starting to stack! Great implementation, this is going to be really fun to use ✨

Comment threadsrc/Dialog/ConfirmationDialog.tsx Outdated
Comment threadsrc/Dialog/ConfirmationDialog.tsx Outdated
Comment threadsrc/Dialog/ConfirmationDialog.tsx
Comment threadsrc/Dialog/ConfirmationDialog.tsx Outdated
Comment threadsrc/Dialog/ConfirmationDialog.tsx
vdepizzol
vdepizzol reviewed Apr 23, 2021
View reviewed changes
Comment threadsrc/Dialog/ConfirmationDialog.tsx Outdated
T-Hugsand others added 3 commits April 23, 2021 12:10
@T-Hugs
T-Hugs merged commit e803170 into mainApr 23, 2021
@T-Hugs
T-Hugs deleted the confirm branch April 23, 2021 16:31
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smocklesmocklesmockle left review comments
+4 more reviewers
@vdepizzolvdepizzolvdepizzol left review comments
@colebemiscolebemiscolebemis left review comments
@VanAndersonVanAndersonVanAnderson approved these changes
@dgreifdgreifdgreif approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Dialog

6 participants

@T-Hugs@vdepizzol@dgreif@smockle@colebemis@VanAnderson