-
Notifications
You must be signed in to change notification settings - Fork 25
fix: bottom drawer nesting issues #1460
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
d1395f6 to
45d9107
Compare
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 refactors the bottom drawer implementation to fix nesting issues by introducing a centralized store for managing drawer state. The previous approach of passing drawer references through props has been replaced with a Pinia store that tracks all active drawers, enabling proper animation handling and simplifying nested menu logic.
Key Changes:
- Introduced a new
useBottomDrawerPinia store to centrally manage drawer lifecycle and state - Removed
NestedDroptype and associated prop-drilling of drawer references throughout the component hierarchy - Simplified drawer nesting by leveraging the store's drawer stack instead of parent-child references
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/design-system/src/composables/stores/bottomDrawer.ts |
New Pinia store managing drawer registration, current drawer tracking, and lifecycle methods |
packages/design-system/src/components/OcBottomDrawer/OcBottomDrawer.vue |
Refactored to use the drawer store instead of manual parent/child reference management |
packages/design-system/src/components/OcDrop/OcDrop.vue |
Removed nested drawer prop passing and simplified exposed API |
packages/design-system/src/helpers/types.ts |
Removed deprecated NestedDrop type definition |
packages/web-pkg/src/components/ContextActions/*.vue |
Removed dropRef props that are no longer needed |
packages/web-app-files/src/views/spaces/*.vue |
Updated context menu slot bindings to remove dropRef |
packages/design-system/package.json |
Added Pinia dependency |
packages/design-system/src/components/OcBottomDrawer/OcBottomDrawer.spec.ts |
Updated tests to mock the new drawer store |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/design-system/src/components/OcBottomDrawer/OcBottomDrawer.vue
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/OcBottomDrawer/OcBottomDrawer.vue
Outdated
Show resolved
Hide resolved
45d9107 to
e218ab9
Compare
|
Noticed a regression:
|
Fixes several issues with the bottom drawer (especially with nested ones) and improves the general animation handling when opening/closing nested menus. This was achieved by introducing a store that keeps track of all the registered bottom drawers.
e218ab9 to
aba40f0
Compare
Good catch, thx! Should be fixed 👍 |
AlexAndBear
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.
Works like a charm 👍
fix: bottom drawer nesting issues
Fixes several issues with the bottom drawer (especially with nested ones) and improves the general animation handling when opening/closing nested menus.
This was achieved by introducing a store that keeps track of all the registered bottom drawers. This should be a much cleaner approach than the previous one.
fixes #1444
fixes #1445
fixes #1451