-
Notifications
You must be signed in to change notification settings - Fork 246
feat(data-modeling): add telemetry and align design COMPASS-9594 #7139
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 adds telemetry tracking for diagram imports and redesigns the export modal and toolbar in the data modeling feature. The changes consolidate the previous separate download and export functionality into a unified export flow with multiple format options.
- Adds telemetry event tracking when users import diagrams from files
- Redesigns the export modal to include three format options (Diagram File, PNG, JSON) with descriptions
- Updates the toolbar design with improved styling and removes the separate download button
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-telemetry/src/telemetry-events.ts | Adds new telemetry event type for diagram imports and extends export format options |
| packages/compass-data-modeling/src/store/diagram.ts | Integrates telemetry tracking in the file import workflow |
| packages/compass-data-modeling/src/store/export-diagram.ts | Adds support for 'diagram' export format and removes duplicate download logic |
| packages/compass-data-modeling/src/components/export-diagram-modal.tsx | Redesigns modal UI with three export options and descriptive text |
| packages/compass-data-modeling/src/components/diagram-editor-toolbar.tsx | Redesigns toolbar with new styling and removes download button |
| packages/compass-components/src/components/icons/png-icon.tsx | Adds new PNG icon component with dark mode support |
| packages/compass-e2e-tests/tests/data-modeling-tab.test.ts | Updates test to use new export modal workflow |
| packages/compass-e2e-tests/helpers/selectors.ts | Updates selectors to reflect UI changes |
Comments suppressed due to low confidence (2)
packages/compass-data-modeling/src/components/diagram-editor-toolbar.tsx:22
- The function name 'getColorWithOpactity' contains a spelling error and should be renamed to 'getColorWithOpacity' for clarity and correctness.
function getColorWithOpactity(color: string, opacity: number) {
packages/compass-data-modeling/src/store/export-diagram.ts:136
- [nitpick] The error message could be more helpful by listing the supported formats: 'Unsupported export format: ${exportFormat}. Supported formats are: png, json, diagram'.
throw new Error(`Unsupported export format: ${exportFormat}`);
packages/compass-data-modeling/src/components/diagram-editor-toolbar.tsx
Outdated
Show resolved
Hide resolved
| /** | ||
| * @param color 6-digit hex color code | ||
| * @param opacity a number between 0 and 1 representing the opacity | ||
| * @returns 8-digit hex color code with the last two digits representing the opacity | ||
| */ | ||
| function getColorWithOpacity(color: string, opacity: number) { | ||
| if (opacity < 0 || opacity > 1) { | ||
| throw new Error('Opacity must be between 0 and 1'); | ||
| } | ||
| const alpha = Math.round(opacity * 255) | ||
| .toString(16) | ||
| .padStart(2, '0'); | ||
| return `#${color.replace('#', '')}${alpha}`; | ||
| } |
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.
We have polished in compass components that has methods for that, just for consistency probably better to re-export needed methods from the package?
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.
added in 5b64696. polished seems nice lib!
gribnoysup
left a comment
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.
One comment for using library that we already depend on, but otherwise looks nice!
| aria-label="Diagram File" | ||
| onClick={() => onSelectFormat('diagram')} | ||
| size="small" | ||
| description="Importable into Compass so teammates can collaborate." |
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.
That's a leafygreen thing, but this description not being clickable is such a weird design decision 😬
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.
agreed, i was thinking of keeping it outside of the radio component for this purposes, but to keep things cleaner, choose this way :)
In this PR added telemetry for importing diagram and aligned export diagram and toolbar design.
Preview
Toolbar
Modal
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes