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

Do not close dialog when clicking on the backdrop when it contains a form #3334

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

klaustopher
Copy link
Contributor

What are you trying to accomplish?

The docs state that dialogs should not be closed when clicking on the backdrop, when the dialog contains a form:

By default, clicking on the backdrop will dismiss the dialog. However, if the dialog contains a form that can have unsaved changes, the backdrop won't dismiss the dialog, regardless of the state of the form.

This behavior was not implemented yet. The PR adds the check to the mouse click handler for the dialog that it is not automatically closed, when the dialog contains a form.

Screenshots

Monosnap.screencast.2025-02-18.09-43-15.mp4

Integration

As this fix implenents behavior that is already documented, IMHO no change to documentation is needed.

List the issues that this change affects.

Fixes #3214

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

As the docs state that regardless of the state of the form the dialog should not be closed, I did not implement any logic to do dirty tracking of the form or similar, instead I just check for existance of a form within the dialog and thus prevent the auto closing logic.

the backdrop won't dismiss the dialog, regardless of the state of the form.

Anything you want to highlight for special attention from reviewers?

no

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@klaustopher klaustopher requested a review from a team as a code owner February 18, 2025 09:16
Copy link

changeset-bot bot commented Feb 18, 2025

🦋 Changeset detected

Latest commit: 11cffd3

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

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

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

@klaustopher klaustopher force-pushed the fix-dialog-outside-click-with-form branch from 1d43a05 to 11cffd3 Compare February 18, 2025 09:29
@joshblack joshblack requested a review from jonrohan February 19, 2025 21:02
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

LGTM but want to confirm with @jonrohan 👀

@HDinger
Copy link
Contributor

HDinger commented Mar 11, 2025

Hi @jonrohan are there any updates on this?

@jonrohan jonrohan merged commit c6996b5 into primer:main Mar 11, 2025
30 of 32 checks passed
@primer primer bot mentioned this pull request Mar 11, 2025
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.

Dialogs with a Form inside are closed on outside click
4 participants