Skip to content

Commit

Permalink
refactor(files): Adjust useNavigation composable to enforce active
Browse files Browse the repository at this point in the history
view

In some cases it is guaranteed that we have a proper active view,
e.g. when the file list is loaded (so within FileEntry* components).
This does not change anything but the Typescript types,
so the `currentView` is typed as `View` instead of `View | null´.

Signed-off-by: Ferdinand Thiessen <[email protected]>
  • Loading branch information
susnux committed Nov 15, 2024
1 parent ca4be91 commit d61be26
Show file tree
Hide file tree
Showing 6 changed files with 32 additions and 24 deletions.
7 changes: 4 additions & 3 deletions apps/files/src/components/FileEntry.vue
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@
<!-- View columns -->
<td v-for="column in columns"
:key="column.id"
:class="`files-list__row-${currentView?.id}-${column.id}`"
:class="`files-list__row-${currentView.id}-${column.id}`"
class="files-list__row-column-custom"
:data-cy-files-list-row-column-custom="column.id"
@click="openDetailsIfAvailable">
Expand Down Expand Up @@ -135,7 +135,8 @@ export default defineComponent({
const filesStore = useFilesStore()
const renamingStore = useRenamingStore()
const selectionStore = useSelectionStore()
const { currentView } = useNavigation()
// The file list is guaranteed to be only shown with active view - thus we can set the `loaded` flag
const { currentView } = useNavigation(true)
const {
directory: currentDir,
fileId: currentFileId,
Expand Down Expand Up @@ -180,7 +181,7 @@ export default defineComponent({
if (this.filesListWidth < 512 || this.compact) {
return []
}
return this.currentView?.columns || []
return this.currentView.columns || []
},
size() {
Expand Down
10 changes: 5 additions & 5 deletions apps/files/src/components/FileEntry/FileEntryActions.vue
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@
</template>
<script lang="ts">
import type { PropType, ShallowRef } from 'vue'
import type { FileAction, Node, View } from '@nextcloud/files'
import type { PropType } from 'vue'
import type { FileAction, Node } from '@nextcloud/files'
import { DefaultType, NodeStatus } from '@nextcloud/files'
import { showError, showSuccess } from '@nextcloud/dialogs'
Expand Down Expand Up @@ -133,12 +133,12 @@ export default defineComponent({
},
setup() {
const { currentView } = useNavigation()
// The file list is guaranteed to be only shown with active view - thus we can set the `loaded` flag
const { currentView } = useNavigation(true)
const enabledFileActions = inject<FileAction[]>('enabledFileActions', [])
return {
// The file list is guaranteed to be only shown with active view
currentView: currentView as ShallowRef<View>,
currentView,
enabledFileActions,
}
},
Expand Down
9 changes: 5 additions & 4 deletions apps/files/src/components/FileEntry/FileEntryName.vue
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ export default defineComponent({
},
setup() {
const { currentView } = useNavigation()
// The file list is guaranteed to be only shown with active view - thus we can set the `loaded` flag
const { currentView } = useNavigation(true)
const { directory } = useRouteParameters()
const renamingStore = useRenamingStore()
Expand Down Expand Up @@ -143,7 +144,7 @@ export default defineComponent({
}
}
if (this.defaultFileAction && this.currentView) {
if (this.defaultFileAction) {
const displayName = this.defaultFileAction.displayName([this.source], this.currentView)
return {
is: 'button',
Expand Down Expand Up @@ -247,8 +248,8 @@ export default defineComponent({
if (status) {
showSuccess(t('files', 'Renamed "{oldName}" to "{newName}"', { oldName, newName }))
this.$nextTick(() => {
const nameContainter = this.$refs.basename as HTMLElement | undefined
nameContainter?.focus()
const nameContainer = this.$refs.basename as HTMLElement | undefined
nameContainer?.focus()
})
} else {
// Was cancelled - meaning the renaming state is just reset
Expand Down
3 changes: 2 additions & 1 deletion apps/files/src/components/FileEntryGrid.vue
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ export default defineComponent({
const filesStore = useFilesStore()
const renamingStore = useRenamingStore()
const selectionStore = useSelectionStore()
const { currentView } = useNavigation()
// The file list is guaranteed to be only shown with active view - thus we can set the `loaded` flag
const { currentView } = useNavigation(true)
const {
directory: currentDir,
fileId: currentFileId,
Expand Down
18 changes: 10 additions & 8 deletions apps/files/src/composables/useNavigation.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
import type { Navigation, View } from '@nextcloud/files'

import { beforeEach, describe, expect, it, vi } from 'vitest'
import { mount } from '@vue/test-utils'
import { defineComponent } from 'vue'

import { useNavigation } from './useNavigation'
import * as nextcloudFiles from '@nextcloud/files'

const { Navigation, View } = nextcloudFiles
const { Navigation: ncNavigation, View: ncView } = nextcloudFiles

// Just a wrapper so we can test the composable
const TestComponent = defineComponent({
Expand All @@ -29,7 +31,7 @@ describe('Composables: useNavigation', () => {

describe('currentView', () => {
beforeEach(() => {
navigation = new Navigation()
navigation = new ncNavigation()

Check failure on line 34 in apps/files/src/composables/useNavigation.spec.ts

View workflow job for this annotation

GitHub Actions / NPM lint

A constructor name should not start with a lowercase letter
spy.mockImplementation(() => navigation)
})

Expand All @@ -39,7 +41,7 @@ describe('Composables: useNavigation', () => {
})

it('should return already active navigation', async () => {
const view = new View({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-1', name: 'My View 1', order: 0 })
const view = new ncView({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-1', name: 'My View 1', order: 0 })

Check failure on line 44 in apps/files/src/composables/useNavigation.spec.ts

View workflow job for this annotation

GitHub Actions / NPM lint

A constructor name should not start with a lowercase letter
navigation.register(view)
navigation.setActive(view)
// Now the navigation is already set it should take the active navigation
Expand All @@ -48,7 +50,7 @@ describe('Composables: useNavigation', () => {
})

it('should be reactive on updating active navigation', async () => {
const view = new View({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-1', name: 'My View 1', order: 0 })
const view = new ncView({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-1', name: 'My View 1', order: 0 })

Check failure on line 53 in apps/files/src/composables/useNavigation.spec.ts

View workflow job for this annotation

GitHub Actions / NPM lint

A constructor name should not start with a lowercase letter
navigation.register(view)
const wrapper = mount(TestComponent)

Expand All @@ -63,7 +65,7 @@ describe('Composables: useNavigation', () => {

describe('views', () => {
beforeEach(() => {
navigation = new Navigation()
navigation = new ncNavigation()

Check failure on line 68 in apps/files/src/composables/useNavigation.spec.ts

View workflow job for this annotation

GitHub Actions / NPM lint

A constructor name should not start with a lowercase letter
spy.mockImplementation(() => navigation)
})

Expand All @@ -73,7 +75,7 @@ describe('Composables: useNavigation', () => {
})

it('should return already registered views', () => {
const view = new View({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-1', name: 'My View 1', order: 0 })
const view = new ncView({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-1', name: 'My View 1', order: 0 })

Check failure on line 78 in apps/files/src/composables/useNavigation.spec.ts

View workflow job for this annotation

GitHub Actions / NPM lint

A constructor name should not start with a lowercase letter
// register before mount
navigation.register(view)
// now mount and check that the view is listed
Expand All @@ -82,8 +84,8 @@ describe('Composables: useNavigation', () => {
})

it('should be reactive on registering new views', () => {
const view = new View({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-1', name: 'My View 1', order: 0 })
const view2 = new View({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-2', name: 'My View 2', order: 1 })
const view = new ncView({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-1', name: 'My View 1', order: 0 })

Check failure on line 87 in apps/files/src/composables/useNavigation.spec.ts

View workflow job for this annotation

GitHub Actions / NPM lint

A constructor name should not start with a lowercase letter
const view2 = new ncView({ getContents: () => Promise.reject(new Error()), icon: '<svg></svg>', id: 'view-2', name: 'My View 2', order: 1 })

Check failure on line 88 in apps/files/src/composables/useNavigation.spec.ts

View workflow job for this annotation

GitHub Actions / NPM lint

A constructor name should not start with a lowercase letter

// register before mount
navigation.register(view)
Expand Down
9 changes: 6 additions & 3 deletions apps/files/src/composables/useNavigation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,21 @@ import { onMounted, onUnmounted, shallowRef, triggerRef } from 'vue'

/**
* Composable to get the currently active files view from the files navigation
* @param _loaded If set enforce a current view is loaded
*/
export function useNavigation() {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export function useNavigation<T extends boolean>(_loaded?: T) {
type MaybeView = T extends true ? View : (View | null);
const navigation = getNavigation()
const views: ShallowRef<View[]> = shallowRef(navigation.views)
const currentView: ShallowRef<View | null> = shallowRef(navigation.active)
const currentView: ShallowRef<MaybeView> = shallowRef(navigation.active as MaybeView)

/**
* Event listener to update the `currentView`
* @param event The update event
*/
function onUpdateActive(event: CustomEvent<View|null>) {
currentView.value = event.detail
currentView.value = event.detail as MaybeView
}

/**
Expand Down

0 comments on commit d61be26

Please sign in to comment.