Skip to content

Conversation

shleewhite
Copy link
Contributor

@shleewhite shleewhite commented Apr 29, 2025

📌 Summary

If merged this PR would update Modal to not use ember-focus-trap to dismiss the Modal on click outside and instead use our own click event.

It also fixes an issue where the scroll bar would get added back if try to close the modal when isDismissDisabled=true.

🛠️ Detailed description

This is necessary because the ember-focus-trap's clickOutSideDeactivates only ever runs once. If a consumer conditionally disables the ability to dismiss the modal, the user may not be able to click outside to deactivate it.

Steps to reproduce:

  1. Navigate to: https://hds-showcase.vercel.app/components/modal#demo
  2. Click "Open non-dismissable modal"
  3. Click outside the modal after it opens, notice it doesn't close and you are able to scroll the page behind the modal.
  4. Click "reset isDismissDisabled"
  5. Click outside the modal, notice it doesnt close.
  6. Click "confirm"/"Cancel"/"close", notice the modal does close.

🔗 External links

Jira ticket: HDS-3972


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Apr 29, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview May 6, 2025 5:24pm
hds-website ✅ Ready (Inspect) Visit Preview May 6, 2025 5:24pm

@didoo
Copy link
Contributor

didoo commented Apr 30, 2025

It's quite a complex PR with potential side-effects. Maybe @RobbieTheWagner could review it too?

@zamoore
Copy link
Contributor

zamoore commented Apr 30, 2025

Have you considered moving the logic out of the modal and into a global on-click-outside modifier? Might be a useful modifier to ship with HDS.

@RobbieTheWagner
Copy link
Member

Have you considered moving the logic out of the modal and into a global on-click-outside modifier? Might be a useful modifier to ship with HDS.

I like this idea. FWIW, there is a well maintained addon for this, if we want to just use that https://github.com/lifeart/ember-click-outside-modifier

@shleewhite
Copy link
Contributor Author

Have you considered moving the logic out of the modal and into a global on-click-outside modifier? Might be a useful modifier to ship with HDS.

I like this idea. FWIW, there is a well maintained addon for this, if we want to just use that https://github.com/lifeart/ember-click-outside-modifier

@RobbieTheWagner @zamoore This is definitely an interesting idea... I think for now because there really is only one use so far I'm leaning towards leaving the one off implementation. We usually rely on ember-focus-trap for this functionality, so I don't want to add another dependency.

Co-authored-by: Melanie Sumner <melanie@hashicorp.com>
dchyun
dchyun previously approved these changes May 6, 2025
Copy link
Contributor

@dchyun dchyun left a comment

Choose a reason for hiding this comment

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

LGTM! Only suggestion, should we add a test case for the document click in the existing isDIsmissDisabled test?

@shleewhite shleewhite merged commit 3dcdc17 into main May 7, 2025
17 checks passed
@shleewhite shleewhite deleted the hds-3972/modal-dismiss branch May 7, 2025 17:02
@hashibot-hds hashibot-hds mentioned this pull request May 7, 2025
didoo added a commit that referenced this pull request Jun 20, 2025
This reverts commit 3dcdc17.

# Conflicts:
#	showcase/tests/integration/components/hds/modal/index-test.js (kept the test)
@didoo didoo mentioned this pull request Jun 20, 2025
5 tasks
didoo added a commit that referenced this pull request Jun 20, 2025
didoo added a commit that referenced this pull request Jun 20, 2025
@hashibot-hds hashibot-hds mentioned this pull request Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants