feat: vendor LG drawer and toolbar COMPASS-9406 #7120
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR vendors the LG Drawer and Toolbar components as a temporary solution until a proper LG upgrade can be performed. The main changes include adding vendored drawer and toolbar components to the compass-components package and updating the compass-data-modeling package to use the new DrawerLayout with toolbar functionality.
- Adds vendored LG Drawer and Toolbar components with custom styling and architecture
- Updates compass-data-modeling to use DrawerLayout instead of custom side panel implementation
- Simplifies side panel component by removing state management and close functionality
Reviewed Changes
Copilot reviewed 58 out of 59 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-data-modeling/src/components/diagram-editor-side-panel.tsx | Simplified side panel component removing state management and close functionality |
| packages/compass-data-modeling/src/components/data-modeling.tsx | Updated to use DrawerLayout with toolbar data for side panel functionality |
| packages/compass-components/src/components/leafygreen.tsx | Added exports for new drawer components |
| packages/compass-components/src/components/drawer/* | Added complete vendored drawer component system with layout, toolbar, and context management |
| packages/compass-components/src/components/toolbar/* | Added complete vendored toolbar component system with icon buttons and context management |
| packages/compass-components/package.json | Added new LG dependencies for descendants and lib utilities |
Comments suppressed due to low confidence (1)
packages/compass-data-modeling/src/components/diagram-editor-side-panel.tsx:44
- The function name was corrected from 'DiagmramEditorSidePanel' to 'DiagramEditorSidePanel' but the connect call still references the old misspelled name. This should be updated to match the corrected function name.
})(DiagramEditorSidePanel);
packages/compass-components/src/components/drawer/Drawer/Drawer.tsx
Outdated
Show resolved
Hide resolved
packages/compass-components/src/components/toolbar/toolbar-icon-button/toolbar-icon-button.tsx
Show resolved
Hide resolved
packages/compass-data-modeling/src/components/diagram-editor-side-panel.tsx
Outdated
Show resolved
Hide resolved
paula-stacho
added
the
feature flagged
There was a problem hiding this comment.
Changes look good. It might be good to add a README or a comment disclaimer somewhere in the components we're adding to compass-components that these are the leafygreen components and to keep that in mind when we want to make changes. We want to avoid them diverging too much as that would make it harder to switch over down the line.
The Toolbar component sounds like a generic one that might get used in other places.
packages/compass-data-modeling/src/components/diagram-editor-side-panel.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Can I selfishly ask not to include these changes in this patch? 😅 I'm changing state quite some in the open PR for the data-modeling drawer state and don't want to deal with extra conflicts there. After we merge it, I can follow-up and add the drawer usage in the package
There was a problem hiding this comment.
Sure, I can clean it up and just include the components changes
| "@leafygreen-ui/emotion": "^4.0.9", | ||
| "@leafygreen-ui/guide-cue": "^7.0.2", | ||
| "@leafygreen-ui/hooks": "^8.3.4", | ||
| "@leafygreen-ui/icon": "^13.1.2", | ||
| "@leafygreen-ui/icon-button": "16.0.2", | ||
| "@leafygreen-ui/info-sprinkle": "^4.0.2", | ||
| "@leafygreen-ui/leafygreen-provider": "^4.0.2", | ||
| "@leafygreen-ui/lib": "^15.2.0", |
There was a problem hiding this comment.
We now have two versions of lib package in compass-components, 14 and 15. Is it required to pull this dependency here at this exact version? If not, it would be better if we can keep it aligned
There was a problem hiding this comment.
Hmmm, looking closer and I think it is already somewhat of an issue due to diagramming package pulling its own leafygreen deps (not only lib, but a bunch of others too):
└─┬ @mongodb-js/[email protected] -> ./packages/compass-data-modeling
└─┬ @mongodb-js/[email protected]
├─┬ @leafygreen-ui/[email protected]
│ ├─┬ @leafygreen-ui/[email protected]
│ │ └── @leafygreen-ui/[email protected] deduped
│ └── @leafygreen-ui/[email protected]
└─┬ @leafygreen-ui/[email protected]
└── @leafygreen-ui/[email protected] deduped
... so if it's easier to keep like that for your patch, that is fine. We might need to deal with this later though, but it can wait
There was a problem hiding this comment.
The vendored packages call it directly, v14 doesn't work for them. I'm hoping we'll do the proper upgrade soon, and for the direct dependency here we'll be reminded by depcheck to clean this up. If we really want to avoid this I think we'd just need to avoid all the new lg-id bits.
There was a problem hiding this comment.
Got it, that's fair, yeah
There was a problem hiding this comment.
One note on including leafygreen-lib in direct deps of compass-components, otherwise looks good.
Also as I mentioned, if this makes sense to you, do you mind not including changes to the data-modeling package in this patch? I think it might be easier to adjust the data-modeling package after both of our changes land (or if this patch lands first, I can just update it in my PR)
Description
As a temporary solution, until we can do the proper LG upgrade - vendoring LG Drawer and LG Toolbar (with style adjustments and tests removed).
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes