fix: close only the topmost modal on Escape#22168
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #22168 +/- ##
=======================================
Coverage 95.10% 95.10%
=======================================
Files 548 548
Lines 45599 45605 +6
Branches 6582 6552 -30
=======================================
+ Hits 43367 43373 +6
Misses 2102 2102
Partials 130 130
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hi @adamalston,
Thanks for taking the time to look into this and for the work you’ve put into the PR.
I just wanted to share a bit of context that might be helpful here — this approach is quite similar to my earlier PR #21436, which we ultimately decided not to move forward with because it relied on a querySelector-based solution.
Following that, we created a research issue (#21650) to explore a more robust ref-based approach using a registration model. That research has since been completed and the issue is now closed with the findings documented.
We’ll be creating a follow-up issue for the implementation soon based on that direction — happy to continue the discussion there.
Following a discussion with the team, we’re okay to proceed with this approach for now. If any issues come up down the line, we can revisit this and consider one of the alternative solutions outlined in the research issue (#21650).
cc: @tay1orjones
|
Appreciate the context. If there's already a preferred direction based on the research in #21650, I'm also fine not proceeding with this pull request and waiting for that implementation instead. |
tay1orjones
left a comment
There was a problem hiding this comment.
I'm not sure relying on DOM order is robust enough since folks could technically portal the node into anywhere. but I think it's a decent stopgap for now as we consider a more robust approach @sangeethababu9223's been thinking about.
Ideally this will all be handled by the browser eventually with native <dialog> so something to just get by with for now seems prudent.
9edbceb
Closes #20426
Closed only the topmost modal on Escape.
Changelog
Changed
Testing / Reviewing
I wasn't sure where to abstract the util to, so I put it in
packages/react/src/components/Modal/isTopmostVisibleModal.ts. If there's a preference to put it inpackages/react/src/internalor elsewhere, let me know.PR Checklist
As the author of this PR, before marking ready for review, confirm you:
Updated documentation and storybook examplesMore details can be found in the pull request guide