Skip to content

Conversation

@brandon-pereira
Copy link
Member

Improves user reported issues that clicking outside the modal would click elements unexpectedly, also improves accessibility (keyboard focus trap & esc to exit)

Fixes HDX-2642

Improves user reported issues that clicking outside the modal would click elements unexpectedly, also improves accessibility (keyboard focus trap & esc to exit)

Fixes HDX-2642
@brandon-pereira brandon-pereira requested review from a team and pulpdrew and removed request for a team October 23, 2025 14:45
@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2025

🦋 Changeset detected

Latest commit: 71a061d

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

This PR includes changesets to release 2 packages
Name Type
@hyperdx/app Patch
@hyperdx/api 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

@vercel
Copy link

vercel bot commented Oct 23, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview Comment Oct 25, 2025 4:44pm

@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2025

E2E Test Results

All tests passed • 26 passed • 3 skipped • 219s

Status Count
✅ Passed 26
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 3

View full report →

@claude
Copy link

claude bot commented Oct 23, 2025

PR Review: Update Drawers to do a proper focus trap

Critical Issues

✅ No critical issues found.

Code Quality Notes

Good changes:

  • ✅ Properly migrated from react-modern-drawer to Mantine Drawer component (follows CLAUDE.md preference for Mantine UI)
  • ✅ Implemented proper focus trap with withCloseButton={false} and keyboard navigation (ESC key handling)
  • ✅ Fixed accessibility issues by preventing unwanted clicks through overlays
  • ✅ Consistent prop naming: open → opened, direction → position (Mantine conventions)
  • ✅ Updated E2E tests to use proper data-testid selectors instead of CSS class selectors
  • ✅ Popover portal fix in DBRowTableFieldWithPopover.tsx prevents z-index issues within drawers

Minor observations:

  • ⚠️ DBRowSidePanel.tsx still uses useClickOutside hook alongside Mantine Drawer built-in onClose - this creates two click-outside handlers. This appears intentional for the custom drag bar feature but worth noting for future reference.
  • ℹ️ All drawers now use withCloseButton={false} and implement custom close buttons, maintaining consistent UI patterns

Summary

This is a clean refactoring that improves accessibility and user experience. The migration to Mantine Drawer is well-executed and aligns with the project architectural guidelines. No blocking issues.

"react-hotkeys-hook": "^4.3.7",
"react-json-tree": "^0.17.0",
"react-markdown": "^8.0.4",
"react-modern-drawer": "^1.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

🤠

position="top-start"
offset={5}
opened={opened}
zIndex={1}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great because I can see the popovers now, but I'm also seeing that the add / exclude filter buttons are not having an effect when in the side drawer other than closing the side drawer:

Screen.Recording.2025-10-25.at.11.36.27.AM.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for testing it! I tested so many things to ensure the zindex worked, but didn't click it 😆 . The issue was the tooltips were rendering in a portal and now with the accessibility improvements, the drawer was auto-closing due to a click outside the modal. I fixed this by ensuring the tooltips render in the same container as the table, thus avoiding the portal click outside logic

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.

2 participants