Add bottom drawer for mobile devices #985
Conversation
AlexAndBear
force-pushed
the
bottom-drawer
branch
from
6a38d47 to
5fd4506
Compare
packages/web-app-files/src/components/SideBar/Shares/Collaborators/EditDropdown.vue
Outdated
Show resolved
Hide resolved
JammingBen
force-pushed
the
bottom-drawer
branch
from
a213825 to
8a5eb39
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds bottom drawer functionality for mobile devices to improve the mobile user experience by replacing dropdown menus with slide-up drawers on small screens. The implementation includes a new OcBottomDrawer component and updates existing dropdown components to automatically use bottom drawers on mobile devices.
- Adds
OcBottomDrawercomponent with mobile-optimized UI patterns - Implements automatic mobile detection to show bottom drawers instead of dropdowns
- Updates multiple dropdown components to support mobile bottom drawer functionality
Reviewed Changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/design-system/src/components/OcBottomDrawer/ | New bottom drawer component with animations, focus trap, and portal support |
| packages/design-system/src/components/OcDrop/OcDrop.vue | Enhanced to automatically show bottom drawer on mobile devices |
| packages/design-system/src/composables/useIsMobile/ | New composable for mobile device detection |
| packages/web-pkg/src/components/SearchBarFilter.vue | Updated to use new filter chip API with proper labeling |
| packages/web-runtime/src/layouts/Application.vue | Added portal target for bottom drawer rendering |
| Multiple component files | Added title props and mobile-specific behavior to dropdown components |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| event: MouseEvent | KeyboardEvent, | ||
| contextMenuButton: ComponentPublicInstance<unknown> | ||
| ) => { | ||
| if (!dropdown) { |
There was a problem hiding this comment.
The null check for dropdown parameter should include proper TypeScript typing. The function signature should indicate that dropdown can be null/undefined, or this check suggests the parameter might be incorrectly typed.
| export const useIsMobile = () => { | ||
| const { width } = useWindowSize() | ||
|
|
||
| const isMobile = computed(() => { | ||
| return width.value < 640 |
There was a problem hiding this comment.
The magic number 640 should be defined as a named constant to improve maintainability and make the breakpoint configurable. Consider defining it as const MOBILE_BREAKPOINT = 640 or using a design system token.
| export const useIsMobile = () => { | |
| const { width } = useWindowSize() | |
| const isMobile = computed(() => { | |
| return width.value < 640 | |
| const MOBILE_BREAKPOINT = 640 | |
| export const useIsMobile = () => { | |
| const { width } = useWindowSize() | |
| const isMobile = computed(() => { | |
| return width.value < MOBILE_BREAKPOINT |
| // we can't use displayPositionedDropdown() on mobile because we need to open the bottom drawer. | ||
| // this can be triggered by clicking the context menu button of the current row. | ||
| const el = document.getElementById(`context-menu-trigger-${item.getDomSelector()}`) | ||
| el?.click() | ||
| return | ||
| } | ||
| displayPositionedDropdown(instance._tippy, event, this.contextMenuButton) | ||
| }, |
There was a problem hiding this comment.
[nitpick] The mobile-specific logic in showContextMenuOnRightClick creates different code paths that could be difficult to test and maintain. Consider extracting this logic into a separate method like handleMobileContextMenu() for better separation of concerns.
| // we can't use displayPositionedDropdown() on mobile because we need to open the bottom drawer. | |
| // this can be triggered by clicking the context menu button of the current row. | |
| const el = document.getElementById(`context-menu-trigger-${item.getDomSelector()}`) | |
| el?.click() | |
| return | |
| } | |
| displayPositionedDropdown(instance._tippy, event, this.contextMenuButton) | |
| }, | |
| this.handleMobileContextMenu(item) | |
| return | |
| } | |
| displayPositionedDropdown(instance._tippy, event, this.contextMenuButton) | |
| }, | |
| handleMobileContextMenu(item: Resource) { | |
| // Handle mobile-specific context menu logic | |
| const el = document.getElementById(`context-menu-trigger-${item.getDomSelector()}`) | |
| el?.click() | |
| }, |
| if (this.isMobile) { | ||
| // we can't use displayPositionedDropdown() on mobile because we need to open the bottom drawer. | ||
| // this can be triggered by clicking the context menu button of the current row. | ||
| const el = document.getElementById(`context-menu-trigger-${item.getDomSelector()}`) |
There was a problem hiding this comment.
Direct DOM manipulation using document.getElementById breaks the Vue reactive paradigm and makes testing difficult. Consider using template refs or emitting an event that the parent component can handle instead.
| // remove active class for the slide-out animation | ||
| const drawer = document.getElementById(drawerId) | ||
| drawer?.classList.remove('active') | ||
| await new Promise((resolve) => setTimeout(resolve, 150)) // wait for the animation to finish |
There was a problem hiding this comment.
The hardcoded 150ms timeout should be defined as a named constant and should match the CSS animation duration to ensure consistency. Consider defining const ANIMATION_DURATION = 150 and using CSS custom properties for the transition timing.
Description
Related Issue
How Has This Been Tested?
Types of changes