Skip to content

Conversation

@AlexAndBear
Copy link
Contributor

@AlexAndBear AlexAndBear commented Jul 28, 2025

Description

image
image

Related Issue

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Types of changes

  • Bugfix
  • Enhancement (a change that doesn't break existing code or deployments)
  • Breaking change (a modification that affects current functionality)
  • Technical debt (addressing code that needs refactoring or improvements)
  • Tests (adding or improving tests)
  • Documentation (updates or additions to documentation)
  • Maintenance (like dependency updates or tooling adjustments)

@JammingBen JammingBen mentioned this pull request Jul 30, 2025
@AlexAndBear AlexAndBear force-pushed the issues/930 branch 2 times, most recently from b9ff1fd to bb926b4 Compare August 4, 2025 11:34
@AlexAndBear AlexAndBear marked this pull request as ready for review August 5, 2025 09:27
Copy link
Contributor

Copilot AI left a 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 drilldown menu functionality for sub-menus on mobile devices by implementing a nested bottom drawer system. The main purpose is to enhance mobile user experience by providing proper navigation between menu levels on touch devices.

  • Implements nested bottom drawer support with parent-child relationships
  • Updates drop menu components to pass dropRef for better mobile integration
  • Adds proper accessibility attributes and navigation controls for nested menus

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/web-runtime/src/layouts/Application.vue Enables multiple portal targets for bottom drawer rendering
packages/web-pkg/src/components/ContextActions/*.vue Updates context action components to support nested drop references
packages/web-pkg/src/components/FilesList/*.vue Passes dropRef through slot props for mobile menu integration
packages/design-system/src/components/OcDrop/OcDrop.vue Refactors to support nested elements with parent references
packages/design-system/src/components/OcBottomDrawer/OcBottomDrawer.vue Implements nested drawer functionality with back navigation
Test snapshots and documentation Updates reflecting the new nested drawer behavior and API changes
Comments suppressed due to low confidence (1)

packages/design-system/src/components/OcDrop/OcDrop.vue:150

  • [nitpick] The prop name 'isNestedElement' is inconsistent with the previous 'isNested' naming pattern. Consider keeping 'isNested' for consistency or updating all related documentation and comments to use 'isNestedElement' terminology consistently.
  isNestedElement = false,

Comment on lines +149 to +155
const bottomDrawerRef = useTemplateRef<HTMLElement | null>('bottomDrawerRef')
const bottomDrawerCardBodyRef = useTemplateRef<HTMLElement | null>('bottomDrawerCardBodyRef')
const open = async () => {
show.value = true
emit('open')
const show = async () => {
if (isNestedElement) {
unref(nestedParentRef).getElement().classList.add('oc-hidden')
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct DOM manipulation using classList should be avoided in Vue components. Consider using Vue's reactive CSS classes or computed properties to manage visibility states instead of directly manipulating the DOM.

Suggested change
const bottomDrawerRef = useTemplateRef<HTMLElement | null>('bottomDrawerRef')
const bottomDrawerCardBodyRef = useTemplateRef<HTMLElement | null>('bottomDrawerCardBodyRef')
const open = async () => {
show.value = true
emit('open')
const show = async () => {
if (isNestedElement) {
unref(nestedParentRef).getElement().classList.add('oc-hidden')
const isParentHidden = ref(false)
const bottomDrawerRef = useTemplateRef<HTMLElement | null>('bottomDrawerRef')
const bottomDrawerCardBodyRef = useTemplateRef<HTMLElement | null>('bottomDrawerCardBodyRef')
const show = async () => {
if (isNestedElement) {
isParentHidden.value = true

Copilot uses AI. Check for mistakes.
unref(bottomDrawerCardBody)?.removeEventListener('click', onBottomDrawerChildClicked)
const openParentDrawer = () => {
hide({ hideParent: false })
unref(nestedParentRef).getElement().classList.remove('oc-hidden')
Copy link

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct DOM manipulation using classList should be avoided in Vue components. Consider using Vue's reactive CSS classes or computed properties to manage visibility states instead of directly manipulating the DOM.

Suggested change
unref(nestedParentRef).getElement().classList.remove('oc-hidden')
unref(nestedParentRef).show()

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some "Gemotze" regarding the aria-labels, but that's not in scope for this PR. More for the bottom drawer in general. I'll open a separate issue about that.

Nice job! UI/UX feels good as well. 🥳 💪

@AlexAndBear AlexAndBear merged commit 2507617 into main Aug 7, 2025
29 checks passed
@AlexAndBear AlexAndBear deleted the issues/930 branch August 7, 2025 12:33
@openclouders openclouders mentioned this pull request Aug 7, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mobile only: add drilldown menu to bottom drawer

3 participants