Skip to content

Trigger onClose when Dialog backdrop is clicked - #4613

Merged
joshblack merged 9 commits into
mainfrom
dg/dialog-backdrop-click
Jun 11, 2024
Merged

Trigger onClose when Dialog backdrop is clicked#4613
joshblack merged 9 commits into
mainfrom
dg/dialog-backdrop-click

Conversation

@joshblack

Copy link
Copy Markdown
Member

Note

This is a re-open of #4565 which was reverted as part of the last release

Relates to https://github.com/github/primer/issues/2531
Relates to https://github.com/github/repos/issues/9248
Alternative to https://github.com/github/primer/issues/1838

I'd like to allow the v2 Dialog to be closed by clicking on the backdrop overlay. This is currently not supported, though the v1 Dialog and PVC dialog support this behavior.

Based on the usage guidelines, there are some unique cases where clicking the backdrop should not close the dialog. By passing in a new backdrop gesture, there is an escape hatch for these unique cases to ignorebackdrop events on a case-by-case basis.

Changelog

New

Dialog and ConfirmationDialog can now be closed by clicking on the backdrop round the dialog. This will cause onClose to be called with a new 'backdrop' gesture.

Changed

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@joshblack
joshblack requested a review from keithamusMay 20, 2024 18:13
@joshblack
joshblack requested a review from a team as a code ownerMay 20, 2024 18:13
@changeset-bot

changeset-botBot commented May 20, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9c35602

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/reactMinor

Not sure what this means? Click here to learn what changesets are.

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

@github-actions

github-actionsBot commented May 20, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
packages/react/dist/browser.esm.js89.46 KB (+0.02% 🔺)
packages/react/dist/browser.umd.js89.73 KB (-0.07% 🔽)

@github-actions
github-actionsBottemporarily deployed to storybook-preview-4613 May 20, 2024 18:17 Inactive
@joshblack

Copy link
Copy Markdown
MemberAuthor

wanted to bump you @keithamus when you have a sec 👀 For context, this was the PR for closing when the backdrop is clicked. You had proposed an alternate way for this but I'm not sure what that would look like. If you have a sec, or want to pair, would appreciate it a ton

@keithamus

Copy link
Copy Markdown
Contributor

The web platform will provide closedby=any (whatwg/html#10157) as the value for closing via a click outside the dialog, and the onclose handler isn't told how the element is closed as there's not a strong use case for being able to differentiate. AIUI the monolith doesn't use the onclose hint that Primer provides and I'm hesitant to extend it especially if it means we're unable to move to the web platform native features which often are more accessible than we could possibly provide (for example allowing for operating system gestures to close the dialog too).

@theinternedtheinterned left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks great to me!

I can certainly see @keithamus argument for staying within the API of the web platform, and honestly I can't think of a reason I'd distinguish between a close button, esc key, and a background click. I could potentially see the need for cancel vs confirm type actions. I suspect in web platform terms the event target could be used to make these distinctions? The same could be done in the react world right?

Still it seems like the removal of "gestures" would be a breaking API change and need not hold up this PR?

@github-actions
github-actionsBottemporarily deployed to storybook-preview-4613 June 11, 2024 15:26 Inactive

@keithamuskeithamus 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.

This LGTM

@github-actions
github-actionsBottemporarily deployed to storybook-preview-4613 June 11, 2024 15:33 Inactive
@joshblack
joshblack enabled auto-merge June 11, 2024 15:34
@github-actions
github-actionsBottemporarily deployed to storybook-preview-4613 June 11, 2024 17:29 Inactive
@joshblack
joshblack added this pull request to the merge queueJun 11, 2024
Merged via the queue into main with commit eb2ab13Jun 11, 2024
@joshblack
joshblack deleted the dg/dialog-backdrop-click branch June 11, 2024 17:52
/**
* This method is invoked when a gesture to close the dialog is used (either
* an Escape key press or clicking the "X" in the top-right corner). The
* an Escape key press, clicking the backdrop, or clicking the "X" in the top-right corner). The

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

should this reference to clicking the backdrop be removed since that information isn't actually communicated?

Sign up for freeto 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.

5 participants

@joshblack@keithamus@theinterned@dwilsonactual@dgreif