feat(NcDialog): New component (moved from `@nextcloud/dialogs`) #4415

susnux · 2023-08-12T15:34:13Z

☑️ Resolves

Fix Filepicker/dialogs component #631
Fix AppSettingsDialog does scroll the section list when clicking on entry #4348 (fixed by migrating AppSettingsDialog to NcDialog base)

Introduces NcDialog as a generic base Dialog component, while NcModal is simply a base for modals like the viewer, NcDialog provides a convenient interface to define dialogs (see example).
The new FilePicker of the @nextcloud/dialogs package is based on top of it.

🖼️ Screenshots

One example dialog:

🚧 Tasks

Maybe migrate to not use <script setup> ? I just copied it, but it would be possible to align with the other components in this library.

🏁 Checklist

⛑️ Tests are included or are not applicable
📘 Component documentation has been extended, updated or is not applicable

…eric dialogs Signed-off-by: Ferdinand Thiessen <[email protected]>

Signed-off-by: Ferdinand Thiessen <[email protected]>

skjnldsv · 2023-08-12T16:52:38Z

src/components/NcRichText/NcRichText.vue

+			const placeholders = this.text.split(/(\{[a-z\-_.0-9]+\})/ig).map((entry) => {
 				const matches = entry.match(/^\{([a-z\-_.0-9]+)\}$/i)
 				// just return plain string nodes as text
 				if (!matches) {
-					return prepareTextNode({ h, context }, entry)
+					return prepareTextNode({ h, context: this }, entry)
 				}
 				// return component instance if argument is an object
 				const argumentId = matches[1]
-				const argument = context.arguments[argumentId]
+				const argument = this.arguments[argumentId]


Looks unrelated

Required to fix eslint errors which would break the CI test. (Comes from introducing Typescript for Vue components).
So I had to either rewrite the code as vanilla Javascript or fix this linting errors.

src/components/NcAppSettingsDialog/NcAppSettingsDialog.vue

szaimen · 2023-08-15T13:30:29Z

@susnux I tested this and seems to work well :)

Can the dialogs be the small size at default? I think this would look much better and would eliminate the need to always overwrite it everywhere :)

szaimen

Tested and the buttons do not show on mobile:

ShGKme

Apart from adding a new component, this PR introduces TS and using SFC Setup. It requires more tooling updates:

Webpack config update to support vue-tsc instead of using ts-loader with tsc and to build TS files
ESLint rules Vue + TS + SFC Setup support
Possibly, tsconfig update

Also, I'm a bit afraid of using SFC Setup in Vue 2. Although it was "backported" at the moment of releasing Vue 2.7, some things were not backported (e.g. Vue 3.3 new stuff) or works differently and has no documentation. And I guess it won't be changed for Vue 2.

What do you think about adding this component first without TS and maybe without SFC Setup but with JS and plain SFC (with Composition API)?

Or, at least after full migration to Vite with another your PR. It will be easier to add first-class TypeScript support 😀

ShGKme · 2023-08-13T21:42:11Z

src/components/NcDialog/NcDialogButton.vue

+	<NcButton :aria-label="props.label" :type="props.type" @click="handleClick">
+		{{ props.label }}
+		<template v-if="props.icon !== undefined" #icon>
+			<component :is="props.icon" :size="20" />
+		</template>
+	</NcButton>


All props are still available directly in SFC Setup's template as in SFC's template (all props are still instance's properties).

Suggested change

<NcButton :aria-label="props.label" :type="props.type" @click="handleClick">

{{ props.label }}

<template v-if="props.icon !== undefined" #icon>

<component :is="props.icon" :size="20" />

</template>

</NcButton>

<NcButton :aria-label="label" :type="type" @click="handleClick">

{{ props.label }}

<template v-if="icon !== undefined" #icon>

<component :is="icon" :size="20" />

</template>

</NcButton>

ShGKme · 2023-08-13T21:46:23Z

src/components/NcDialog/NcDialog.vue

+const emit = defineEmits<{
+	/** Emitted when the dialog is about to close but the out transition did not finished yet */
+	(e: 'closing'): void
+	/** Emitted when the open state changed */
+	(e: 'update:open', v: boolean): void
+}>()


I don't remember if it is supported in Vue 2.7 SFC Setup, but in the latest SFC setup compiler there is a short syntax for defineEmits compile macros.

Suggested change

const emit = defineEmits<{

/** Emitted when the dialog is about to close but the out transition did not finished yet */

(e: 'closing'): void

/** Emitted when the open state changed */

(e: 'update:open', v: boolean): void

}>()

const emit = defineEmits<{

/** Emitted when the dialog is about to close but the out transition did not finished yet */

'closing': []

/** Emitted when the open state changed */

'update:open': [v: boolean]

}>()

ShGKme · 2023-08-15T13:45:48Z

webpack.config.js

+// Support vue + Typescript
+webpackRules.RULE_TS.use = ['babel-loader', { loader: 'ts-loader', options: { appendTsSuffixTo: [/\.vue$/] } }]


ts-loader is no longer recommended way to use TS with Vue when TS is used in templates or with SFC Setup. ts-loader is using tsc to compile TS in the <script> and check types after SRC compiling.

It works, but with limitations, and is different from IDE type-checking by vue-tsc and with Vite build.

Instead, we can compile TS without tsc and check types in a separate script. For example, with babel/preset-typescript which is already added to the config.

Or we can fully move to Vite by merging your PR :)

Then ESBuild instead of tsc will be used to transpile ts.

ShGKme · 2023-08-15T15:16:29Z

tsconfig.json

If we use Vue with TS now, we should also add

"vueCompilerOptions": { "target": 2.7, }

And add @vue/runtime-dom as a dev dependency.

And if I'm not wrong: "./src/**/*.vue" to the include

ShGKme · 2023-08-15T15:17:24Z

src/components/vue-shims.d.ts

Adding vue-shims.d.ts is not a solution for .vue files support in TS/IDE. With this file, by default go-to-declaration in VSCode goes to this file saying "any .vue file is just Vue".

See: https://github.com/vuejs/language-tools/tree/master/packages/vscode-vue#usage

P.S. Works fine in JetBrains

ShGKme · 2023-08-15T15:33:18Z

webpack.config.js

glob in entrypoints doesn't include .ts files, so NcDialog is not included into cjs bundle.

ShGKme · 2023-08-15T15:40:52Z

.eslintrc.js

@@ -9,11 +12,21 @@ module.exports = {
 		appVersion: true,
 	},
 	extends: [
-		'@nextcloud',
+		'@nextcloud/eslint-config/typescript',


It seems that this config is for .ts files only. We need to update @nextcloud/eslint-config to support Vue + TS with SFC Setup.

Also, npm run lint doesn't check TS files.

ShGKme · 2023-08-15T15:58:41Z

src/components/NcDialog/NcDialog.vue

+
+import { useElementSize } from '@vueuse/core'
+import { computed, ref, useSlots } from 'vue'
+import { Fragment } from 'vue-frag' // can be dropped with vue3


Why do we need a <Fragment> here? Slot content doesn't have to have a single root element.

ShGKme · 2023-08-15T21:20:52Z

src/components/NcDialog/NcDialog.vue

+
+const slots = useSlots()


I don't remember if it is supported in Vue 2.7, but in SFC Setup there is a defineSlots macro to define slots with TS.

marcoambrosini

Hi, could with default with a size small or normal for the modal?

susnux · 2023-09-20T15:36:51Z

Close in favor of #4550

susnux added 3. to review Waiting for reviews feature: dialog Related to the dialog component labels Aug 12, 2023

susnux requested review from raimund-schluessler, skjnldsv, ShGKme and hamza221 August 12, 2023 15:34

susnux force-pushed the feat/nc-dialog branch from 13f1423 to b0004e4 Compare August 12, 2023 15:48

susnux added 3 commits August 12, 2023 17:55

feat(NcDialog): Add new NcDialog and NcDialogButton component for gen…

4f3e7ca

…eric dialogs Signed-off-by: Ferdinand Thiessen <[email protected]>

fix(lint): Fix issues with eslint caused by using Typescript in Vue

9bef793

Signed-off-by: Ferdinand Thiessen <[email protected]>

feat(NcAppSettingsDialog): Migrate to use NcDialog as base component

2ad1bf7

Signed-off-by: Ferdinand Thiessen <[email protected]>

susnux force-pushed the feat/nc-dialog branch from b0004e4 to 2ad1bf7 Compare August 12, 2023 15:55

skjnldsv reviewed Aug 12, 2023

View reviewed changes

szaimen reviewed Aug 15, 2023

View reviewed changes

src/components/NcAppSettingsDialog/NcAppSettingsDialog.vue Show resolved Hide resolved

susnux requested review from szaimen and skjnldsv August 15, 2023 13:14

szaimen requested changes Aug 15, 2023

View reviewed changes

ShGKme reviewed Aug 15, 2023

View reviewed changes

marcoambrosini reviewed Aug 16, 2023

View reviewed changes

susnux added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 16, 2023

susnux marked this pull request as draft August 16, 2023 21:24

susnux mentioned this pull request Aug 16, 2023

Migrate build of single entry points from webpack to vite #4386

Merged

2 tasks

susnux mentioned this pull request Sep 20, 2023

feat(NcDialog): New component (moved from @nextcloud/dialogs) #4550

Merged

2 tasks

susnux closed this Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NcDialog): New component (moved from `@nextcloud/dialogs`) #4415

feat(NcDialog): New component (moved from `@nextcloud/dialogs`) #4415

susnux commented Aug 12, 2023 •

edited

Loading

skjnldsv Aug 12, 2023

susnux Aug 14, 2023

szaimen commented Aug 15, 2023

szaimen left a comment

ShGKme left a comment •

edited

Loading

ShGKme Aug 13, 2023

ShGKme Aug 13, 2023 •

edited

Loading

ShGKme Aug 15, 2023

ShGKme Aug 15, 2023

ShGKme Aug 15, 2023

ShGKme Aug 15, 2023

ShGKme Aug 15, 2023

ShGKme Aug 15, 2023

ShGKme Aug 15, 2023

ShGKme Aug 15, 2023

ShGKme Aug 15, 2023

ShGKme Aug 15, 2023

marcoambrosini left a comment

susnux commented Sep 20, 2023

		// Support vue + Typescript
		webpackRules.RULE_TS.use = ['babel-loader', { loader: 'ts-loader', options: { appendTsSuffixTo: [/\.vue$/] } }]

feat(NcDialog): New component (moved from @nextcloud/dialogs) #4415

feat(NcDialog): New component (moved from @nextcloud/dialogs) #4415

Conversation

susnux commented Aug 12, 2023 • edited Loading

☑️ Resolves

🖼️ Screenshots

🚧 Tasks

🏁 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szaimen commented Aug 15, 2023

szaimen left a comment

Choose a reason for hiding this comment

ShGKme left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShGKme Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoambrosini left a comment

Choose a reason for hiding this comment

susnux commented Sep 20, 2023

feat(NcDialog): New component (moved from `@nextcloud/dialogs`) #4415

feat(NcDialog): New component (moved from `@nextcloud/dialogs`) #4415

susnux commented Aug 12, 2023 •

edited

Loading

ShGKme left a comment •

edited

Loading

ShGKme Aug 13, 2023 •

edited

Loading