refactor(files): Adjust some Typescript code in virtual files list #45585

susnux · 2024-05-30T16:33:08Z

Summary

Correctly cast table ref to fix TypeScript errors (I think this also makes it a bit better readable :) )
Refactor the filesListWidth mixin
Use defineComponent to properly inherit typings, also add the $resizeOberserver through the setup to the component instance.
filesListWidth is now a number which defaults to 0, making compares like this.fileListWidth < 768 valid in Typescript (there are a lot of Type error in other places because of this).
Properly add type information to FilesListTableFooter
- Add correct type for nodes prop.
- Use defineComponent to properly infer Typescript information
- Correct usage of nullish coalescing operator
Make currentView a prop of FilesListTableFooter
We already have this as a required prop in FilesListVirtual, so passing it to the footer is straight forward and removed need of computed value.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

susnux

Added some hints about the problems I try to solve

susnux · 2024-05-30T16:34:08Z

apps/files/src/components/FilesListTableFooter.vue

 	props: {
+		currentView: {


susnux · 2024-05-30T16:34:37Z

apps/files/src/components/FilesListTableFooter.vue

@@ -123,7 +123,7 @@ export default Vue.extend({
 			}

 			// Otherwise let's compute it
-			return formatFileSize(this.nodes.reduce((total, node) => total + node.size || 0, 0), true)
+			return formatFileSize(this.nodes.reduce((total, node) => total + (node.size ?? 0), 0), true)


susnux · 2024-05-30T16:36:24Z

apps/files/src/components/FilesListTableFooter.vue

@@ -104,7 +104,7 @@ export default Vue.extend({
 			if (this.dir === '/') {
 				return this.filesStore.getRoot(this.currentView.id)
 			}
-			const fileId = this.pathsStore.getPath(this.currentView.id, this.dir)
+			const fileId = this.pathsStore.getPath(this.currentView.id, this.dir)!


We can safe cast here, if the dir is not found we will get the empty content message instead of the table.
So we can always assume that fileid is not undefined.

So resolving

susnux · 2024-05-30T16:37:03Z

apps/files/src/components/FilesListVirtual.vue

@@ -280,18 +281,19 @@ export default defineComponent({
 			event.preventDefault()
 			event.stopPropagation()

-			const tableTop = this.$refs.table.$el.getBoundingClientRect().top
-			const tableBottom = tableTop + this.$refs.table.$el.getBoundingClientRect().height
+			const tableElement = (this.$refs.table as ComponentPublicInstance<typeof VirtualList>).$el


Resolving

and

susnux · 2024-05-30T16:38:47Z

apps/files/src/components/FilesListTableFooter.vue

@@ -64,7 +68,7 @@ export default Vue.extend({
 			default: false,
 		},
 		nodes: {
-			type: Array,
+			type: Array as PropType<Node[]>,


susnux · 2024-05-30T16:38:59Z

apps/files/src/components/FilesListVirtual.vue

 import type { UserConfig } from '../types'

 import { getFileListHeaders, Folder, View, getFileActions, FileType } from '@nextcloud/files'
 import { showError } from '@nextcloud/dialogs'
 import { loadState } from '@nextcloud/initial-state'
-import { translate as t, translatePlural as n } from '@nextcloud/l10n'


was never used

susnux · 2024-05-30T16:40:16Z

apps/files/src/mixins/filesListWidth.ts

 	data() {
 		return {
-			filesListWidth: null as number | null,
+			filesListWidth: 0,


susnux · 2024-05-30T17:33:53Z

apps/files/src/mixins/filesListWidth.ts


+		// @ts-expect-error The resize observer is just now attached to the object


We can not use setup in mixins, so we can not attach it properly with type information.
Using data also feels wrong, it should not be made reactive.

This mixin is quite small and only defines one property. Maybe we can rewrite it to a composable then?

Yes I also was thinking about that.
Also a composable for dir and fileid makes sense as we use it in many places and just duplicate logic there.

ShGKme · 2024-05-31T10:46:23Z

Thanks for detailed description about all the changes!

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

Use `defineComponent` to properly inherit typings. Expect TS errors for the `$resizeOberserver` as we attach it directly on the component instance. `filesListWidth` is now a number which defaults to 0, making compares like `this.fileListWidth < 768` valid in Typescript. Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

- Add correct type for `nodes` prop. - Use `defineComponent` to properly infer Typescript information - Correct usage of nullish coalescing operator Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

We already have this as a required prop in `FilesListVirtual`, so passing it to the footer is straight forward and removed need of computed value. Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

susnux added 3. to review Waiting for reviews feature: files labels May 30, 2024

susnux added this to the Nextcloud 30 milestone May 30, 2024

susnux requested review from artonge, Pytal and ShGKme May 30, 2024 16:33

susnux requested a review from skjnldsv as a code owner May 30, 2024 16:33

susnux commented May 30, 2024

View reviewed changes

susnux force-pushed the fix/files-filelist-table branch from 9629adf to ed71a9d Compare May 30, 2024 17:32

susnux commented May 30, 2024

View reviewed changes

ShGKme approved these changes May 31, 2024

View reviewed changes

skjnldsv approved these changes May 31, 2024

View reviewed changes

susnux added 5 commits May 31, 2024 17:17

refactor(files): Correctly cast table ref to fix TypeScript errors

0cb3288

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

refactor(files): Properly add type information to FilesListTableFooter

e8adb97

- Add correct type for `nodes` prop. - Use `defineComponent` to properly infer Typescript information - Correct usage of nullish coalescing operator Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

refactor(files): Make currentView a prop of FilesListTableFooter

ceafe43

We already have this as a required prop in `FilesListVirtual`, so passing it to the footer is straight forward and removed need of computed value. Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

chore: Compile assets

8e296a5

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

susnux force-pushed the fix/files-filelist-table branch from ed71a9d to 8e296a5 Compare May 31, 2024 15:20

susnux merged commit 8ed3604 into master May 31, 2024
104 checks passed

susnux deleted the fix/files-filelist-table branch May 31, 2024 16:10

blizzz mentioned this pull request Jul 24, 2024

30.0.0 beta 1 #46713

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(files): Adjust some Typescript code in virtual files list #45585

refactor(files): Adjust some Typescript code in virtual files list #45585

susnux commented May 30, 2024

susnux left a comment

susnux May 30, 2024

susnux May 30, 2024

susnux May 30, 2024

susnux May 30, 2024

susnux May 30, 2024

susnux May 30, 2024

susnux May 30, 2024

susnux May 30, 2024

ShGKme May 31, 2024

susnux May 31, 2024

ShGKme commented May 31, 2024


		// @ts-expect-error The resize observer is just now attached to the object

refactor(files): Adjust some Typescript code in virtual files list #45585

refactor(files): Adjust some Typescript code in virtual files list #45585

Conversation

susnux commented May 30, 2024

Summary

Checklist

susnux left a 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

Choose a reason for hiding this comment

ShGKme commented May 31, 2024