-
Notifications
You must be signed in to change notification settings - Fork 246
fix: clarify import functionality COMPASS-9833 #7649
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR updates the naming of the diagram export format from "Diagram File" to "MDM File" to align with Figma designs. The change improves clarity by using the more specific MDM (MongoDB Data Model) file type designation throughout the UI and test selectors.
- Updated export modal UI labels and aria-labels from "Diagram File" to "MDM File"
- Updated E2E test selector to match the new aria-label
- Added tooltip to import button clarifying that only MDM files from Compass or Atlas Data Explorer can be imported
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/compass-e2e-tests/helpers/selectors.ts | Updated test selector to match new "MDM File" aria-label |
| packages/compass-data-modeling/src/components/import-diagram-button.tsx | Added tooltip explaining MDM file import restrictions |
| packages/compass-data-modeling/src/components/export-diagram-modal.tsx | Changed "Diagram File" labels to "MDM File" in export modal |
| <Tooltip | ||
| trigger={ | ||
| <span> |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrapping <span> prevents proper keyboard navigation and focus management for the button inside. Consider using the disabled prop pattern or removing the span wrapper to maintain accessibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The span is important, we have two triggers on that btn and it gets messed up otherwise. I checked the keyboard navigation and it works fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a div element with inline styles work? span is usually for text content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would work the same, sure, but is it for text content? According to mdn,
<span>is very much like a<div>element, but<div>is a block-level element whereas a<span>is an inline-level element.
https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/span
256b9ee to
3d06df5
Compare
Description
As per figma designs: https://www.figma.com/design/nz84lFaVMLqq9ih4ZJtuP6/Data-modeling-viz-tool?node-id=4184-55423&m=dev
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes