Skip to content
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

feat: add useDraggable composable #4486

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

TFX-98
Copy link
Collaborator

@TFX-98 TFX-98 commented Mar 3, 2025

Description

  1. Closes Draggable and resizeable options for Modal #4422
  2. Added useDraggable composable
  3. Added stories for VaModal
  4. Added stories for useDraggable
  5. Update Docs for VaModal
  6. Update VaModal component with provide draggable logic

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

@TFX-98 TFX-98 added the feature Something useful to end user label Mar 3, 2025
@TFX-98 TFX-98 requested review from asvae and m0ksem March 3, 2025 08:40
@TFX-98 TFX-98 self-assigned this Mar 3, 2025
Copy link

netlify bot commented Mar 3, 2025

👷 Deploy request for vuestic-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 270b5a0

Copy link

netlify bot commented Mar 3, 2025

Deploy Preview for vuestic-storybook ready!

Name Link
🔨 Latest commit 270b5a0
🔍 Latest deploy log https://app.netlify.com/sites/vuestic-storybook/deploys/67c57124466858000897ee8b
😎 Deploy Preview https://deploy-preview-4486--vuestic-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@TFX-98 TFX-98 changed the title add useDraggable composable feat: add useDraggable composable Mar 3, 2025
@TFX-98 TFX-98 force-pushed the feature/draggable-composable branch from 0d48bd9 to 270b5a0 Compare March 3, 2025 09:06
@@ -38,6 +38,8 @@ const modalOptions: Record<keyof ModalOptions, string> = {
overlay: "boolean",
overlayOpacity: "number | string",
zIndex: "number | string",
draggable: "boolean",
draggablePosition: "{ x?: number | string, y?: number | string }",
Copy link
Collaborator Author

@TFX-98 TFX-98 Mar 3, 2025

Choose a reason for hiding this comment

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

May be better to set type like number | '${number}%'?

Copy link
Member

@asvae asvae left a comment

Choose a reason for hiding this comment

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

Let's move remainders in separate issue and merge this.

What I recall:

  • Composable docs are missing.
  • Touch events are not working.
  • There is a bug with compiled docs attachEvent

Copy link
Member

@asvae asvae left a comment

Choose a reason for hiding this comment

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

Let's move remainders in separate issue and merge this.

What I recall:

  • Composable docs are missing.
  • Touch events are not working.
  • There is a bug with compiled docs attachEvent
  • Interaction tests + check rotate font

@@ -422,12 +422,44 @@ watch(isTopLevelModal, newIsTopLevelModal => {
}
}, { immediate: true })

const initialPosition = toRef(props, 'draggablePosition')

const containerRef = ref<HTMLElement | null>(null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const containerRef = ref<HTMLElement | null>(null)
const containerRef = useSelectorTemplateRef(toRef(props,'attachElement'))

const { positionStyle, startDrag } = useDraggable({
draggableRef: modalDialog,
containerRef,
isDraggable: props.draggable,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
isDraggable: props.draggable,
isDraggable: toRef(props, 'draggable'),

stopDrag()
})

watch(position, () => {
Copy link
Collaborator

@m0ksem m0ksem Mar 8, 2025

Choose a reason for hiding this comment

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

PositionStyle can be computed.

Copy link
Collaborator

@m0ksem m0ksem left a comment

Choose a reason for hiding this comment

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

VaModal vs VaDialog

Modal position is broken. Draggable must be applied to the dialog window, instead of overlay element.
image

The purpose of a modal window is to block the application until it is closed. Therefore, there is no reason for modal window to be draggable actually. Instead, we need a dialog window that wouldn't block the application and can be dragged over the page.

I'd not use a draggable prop in VaModal, but instead have it in the VaDialog component, which seems more reasonable. In such case, draggable dialog is more of a popup.

// TODO: add tests
type PositionType = number | `${number}%`
type initialPositionType = { x?: PositionType; y?: PositionType }
export const DraggableAndAttachElement: StoryFn = () => ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const DraggableAndAttachElement: StoryFn = () => ({
export const DraggableAndAttachElement = defineStory({
  • tests

@m0ksem
Copy link
Collaborator

m0ksem commented Mar 8, 2025

useDraggable api

Would be also nice to have no-return api for useDraggable and expose it to end user.

const dialogRef = ref()
const draggableArea = computed(() => dialogRef.value.querySelector('.va-dialog-header'))
useDraggable(dialogRef, {
  draggableChild: draggableArea
})
<VaDialog ref="dialogRef" />

This way, useDraggable must apply position style to dialogRef.
This will be helpful when optimizing the framework for LLMs and Visual Editors. It also removes the extra step of binding styles - if you have multiple draggable windows, you need to rename positionStyle.
See useInputMask, useStickyTableHeader.

This can be considered as step 2 and is not required right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Something useful to end user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Draggable and resizeable options for Modal
3 participants