Skip to content

Conversation

@raimund-schluessler
Copy link

No description provided.

@raimund-schluessler raimund-schluessler force-pushed the vue3 branch 3 times, most recently from 53cadea to 2a77b56 Compare January 28, 2024 21:36
@raimund-schluessler
Copy link
Author

@susnux I worked a bit on the migration, but I could use help with the Typescript stuff.

@susnux
Copy link
Contributor

susnux commented Feb 20, 2024

I could use help with the Typescript stuff.

I will have a look - for the moment I rebased it and fixed some minor issues.

@Antreesy
Copy link
Contributor

Hi @susnux!
i rebased the branch and tried to resolve conflicts from recent app changes. TSC and tests are failing, so I might have not done it correctly 🙈.
Could you double-check, when you have time?

Antreesy and others added 3 commits January 26, 2025 01:11
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme
Copy link
Contributor

ShGKme commented Jan 26, 2025

Pushed an update:

  • Made dependencies inline with main
  • Updated @nextcloud/vue to alpha.6
  • Fixed breaking change in imports

@ShGKme ShGKme self-assigned this Mar 26, 2025
@ShGKme
Copy link
Contributor

ShGKme commented Mar 26, 2025

@raimund-schluessler I'll take over the PR to prepare the release soon, if it's ok

@raimund-schluessler
Copy link
Author

Great, please go ahead!

Copy link

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Seems to work otherwise.

Comment on lines 25 to 31
h(dialog, {
props,
on: {
close: (...rest: unknown[]) => {
onClose(...rest.map(v => toRaw(v)))
vue.$destroy()
},
onClose: (...rest: unknown[]) => {
onClose(...rest.map(v => toRaw(v)))
vue.unmount()
},
}),
Copy link

@st3iny st3iny Apr 3, 2025

Choose a reason for hiding this comment

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

AFAIK, this won't work in Vue 3. We can only pass propsData as the second argument. We need to find another workaround to listen to the close event or simply remove the onClose() handler.

We also need to mount the component explicitly here.

My proposed solution:

	const vue = createApp(dialog, props)
	vue.mount(el)

Copy link
Contributor

Choose a reason for hiding this comment

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

The latest version isn't pushed yet, waiting for the nextcloud-vue release.

Also, it (ideally) relies on spawnDialog from nextcloud-vue.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can only pass propsData as the second argument

This is true for createApp().mount(), not for h

Problem here is that props should be destructured.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to find another workaround to listen to the close event or simply remove the onClose() handler.

In Vue 3 event handlers always passed as onEven param.

<comp @event="handler" /> is h(comp, { onEvent: handler })

Copy link

Choose a reason for hiding this comment

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

TIL. I wasn't aware of the advanced features of h.

Copy link
Contributor

Choose a reason for hiding this comment

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

for dialogs library this createApp is fine, but for nextcloud-vue it can cause unwanted behavior:

  • it does not allow globally registered components in the dialog
  • it does not allow to access registered pinia

Use case would be e.g. a settings store for dialog you spawn with API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants