vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

feat(link shares): allows switching the initial view by passing a query parameter #2335

Merged
konrad merged 11 commits from feature/redirect-to-specific-view into main 2022-09-14 16:37:56 +00:00
Conversation 10 Commits 11 Files Changed 4 +90 -48
4 changed files with 90 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

115
src/components/sharing/linkSharing.vue
View File

@ -79,30 +79,59 @@
>
<thead>
<tr>
<th>{{ $t('list.share.attributes.link') }}</th>
<th>{{ $t('list.share.attributes.name') }}</th>
<th>{{ $t('list.share.attributes.sharedBy') }}</th>
<th>{{ $t('list.share.attributes.right') }}</th>
<th></th>
<th>{{ $t('list.share.links.view') }}</th>
konrad marked this conversation as resolved Outdated
dpschen commented 2022-09-08 09:10:22 +00:00
Outdated
Review
Copy Link

Why is the &nbsp; necessary?

Why is the `&nbsp;` necessary?
konrad commented 2022-09-08 11:59:13 +00:00
Outdated
Review
Copy Link

I thought it was necessary, looks like it isn't.

I thought it was necessary, looks like it isn't.
👍 1
<th>{{ $t('list.share.attributes.delete') }}</th>
</tr>
</thead>
<tbody>
<tr :key="s.id" v-for="s in linkShares">
<td>
<p class="mb-2 is-italic" v-if="s.name !== ''">
{{ s.name }}
</p>
<p class="mb-2">
<i18n-t keypath="list.share.links.sharedBy">
<strong>{{ s.sharedBy.getDisplayName() }}</strong>
</i18n-t>
</p>
<p class="mb-2">
<template v-if="s.right === RIGHTS.ADMIN">
<span class="icon is-small">
<icon icon="lock"/>
</span>&nbsp;
{{ $t('list.share.right.admin') }}
</template>
<template v-else-if="s.right === RIGHTS.READ_WRITE">
<span class="icon is-small">
<icon icon="pen"/>
</span>&nbsp;
{{ $t('list.share.right.readWrite') }}
</template>
<template v-else>
<span class="icon is-small">
<icon icon="users"/>
</span>&nbsp;
{{ $t('list.share.right.read') }}
</template>
</p>
<div class="field has-addons no-input-mobile">
<div class="control">
<input
:value="getShareLink(s.hash)"
class="input"
readonly
type="text"
:value="getShareLink(s.hash, selectedView[s.id])"
class="input"
readonly
type="text"
/>
</div>
<div class="control">
<x-button
@click="copy(getShareLink(s.hash))"
:shadow="false"
v-tooltip="$t('misc.copy')"
@click="copy(getShareLink(s.hash, selectedView[s.id]))"
:shadow="false"
v-tooltip="$t('misc.copy')"
>
<span class="icon">
<icon icon="paste"/>
@ -112,33 +141,16 @@
</div>
</td>
<td>
<template v-if="s.name !== ''">
{{ s.name }}
</template>
<i v-else>{{ $t('list.share.links.noName') }}</i>
</td>
<td>
{{ s.sharedBy.getDisplayName() }}
</td>
<td class="type">
<template v-if="s.right === RIGHTS.ADMIN">
<span class="icon is-small">
<icon icon="lock"/>
</span>&nbsp;
{{ $t('list.share.right.admin') }}
</template>
<template v-else-if="s.right === RIGHTS.READ_WRITE">
<span class="icon is-small">
<icon icon="pen"/>
</span>&nbsp;
{{ $t('list.share.right.readWrite') }}
</template>
<template v-else>
<span class="icon is-small">
<icon icon="users"/>
</span>&nbsp;
{{ $t('list.share.right.read') }}
</template>
<div class="select">
<select v-model="selectedView[s.id]">
<option
v-for="(title, key) in availableViews"
konrad marked this conversation as resolved Outdated
dpschen commented 2022-09-08 12:21:29 +00:00
Outdated
Review
Copy Link

The id seems unused

The id seems unused
konrad commented 2022-09-08 14:27:13 +00:00
Outdated
Review
Copy Link

Removed it. Was a leftover from an earlier version.

Removed it. Was a leftover from an earlier version.
👍 1
:value="key"
konrad marked this conversation as resolved Outdated
dpschen commented 2022-09-08 12:22:31 +00:00
Outdated
Review
Copy Link

Create the options with a computed base on the LIST_VIEWS. This way the options will always stay up-to-date in case we add a new view.

Create the options with a computed base on the LIST_VIEWS. This way the options will always stay up-to-date in case we add a new view.
konrad commented 2022-09-08 14:29:36 +00:00
Outdated
Review
Copy Link

Done.

Done.
dpschen commented 2022-09-08 14:35:22 +00:00
Outdated
Review
Copy Link

We should try to prevent dynamic message compilation.
See: https://vue-i18n.intlify.dev/guide/advanced/optimization.html#optimization

That's what I meant here: #2335 (comment)

We should try to prevent dynamic message compilation. See: https://vue-i18n.intlify.dev/guide/advanced/optimization.html#optimization That's what I meant here: https://kolaente.dev/vikunja/frontend/pulls/2335#issuecomment-34925
konrad commented 2022-09-08 14:49:51 +00:00
Outdated
Review
Copy Link

How would we do this then? Compute an object with the final messages?

How would we do this then? Compute an object with the final messages?
👍 1
dpschen commented 2022-09-13 15:05:20 +00:00
Outdated
Review
Copy Link

Sorry overread your answer.

Now thinking about it we cannot fullfill both goals (making the type of views update automatically and prevent dynamic message compilation). I would opt for what I said initially. Having a configurable option map:

export const TASK_VIEW_OPTION_MAP : Record<TaskView, string> = {
  list: t('list.list.title'),
  gantt: t('list.gantt.title'),
  table: t('list.table.title'),
  kanban: t('list.kanban.title'),
}

By making the key from type TaskView we prevent at least misuse.
We need to rewrite this to a computed though so that it updates when the language changes.

Sorry overread your answer. Now thinking about it we cannot fullfill both goals (making the type of views update automatically and prevent dynamic message compilation). I would opt for what I said initially. Having a configurable option map: ```ts export const TASK_VIEW_OPTION_MAP : Record<TaskView, string> = { list: t('list.list.title'), gantt: t('list.gantt.title'), table: t('list.table.title'), kanban: t('list.kanban.title'), } ``` By making the key from type `TaskView` we prevent at least misuse. We need to rewrite this to a computed though so that it updates when the language changes.
konrad commented 2022-09-13 20:05:18 +00:00
Outdated
Review
Copy Link

Here's what I came up with: e67fc7fb7e

That might not be the most ideal solution but is seems to work pretty good.

Here's what I came up with: https://kolaente.dev/vikunja/frontend/commit/e67fc7fb7e1678b1b691fee77d3237b222ad50c6 That might not be the most ideal solution but is seems to work pretty good.
👍 1
dpschen commented 2022-09-14 05:39:03 +00:00
Outdated
Review
Copy Link

I think that’s the way to go.

Picky: use an explaining variable name for the label and key in the template

I think that’s the way to go. Picky: use an explaining variable name for the label and key in the template
👍 1
:key="key">
{{ title }}
</option>
</select>
</div>
</td>
<td class="actions">
<x-button
@ -166,7 +178,7 @@
<template #header>
<span>{{ $t('list.share.links.remove') }}</span>
</template>
<template #text>
<p>{{ $t('list.share.links.removeText') }}</p>
</template>
@ -190,6 +202,8 @@ import LinkShareService from '@/services/linkShare'
import {useCopyToClipboard} from '@/composables/useCopyToClipboard'
import {success} from '@/message'
import type {ListView} from '@/types/ListView'
import {LIST_VIEWS} from '@/types/ListView'
const props = defineProps({
listId: {
@ -209,6 +223,17 @@ const showDeleteModal = ref(false)
const linkIdToDelete = ref(0)
const showNewForm = ref(false)
type SelectedViewMapper = Record<IList['id'], ListView>
const selectedView = ref<SelectedViewMapper>({})
const availableViews = computed<Record<ListView, string>>(() => ({
konrad marked this conversation as resolved Outdated
dpschen commented 2022-09-08 09:15:58 +00:00
Outdated
Review
Copy Link

Use Record with IList['id'].

I think in the future even if our ids are defined as numbers in the api it might make sense to cast them as strings in the frontend.

I got the idea while reading the zod documentation.
See: https://github.com/colinhacks/zod#record-key-type under A note on numerical keys.

We should define the types of possible views only once.
Something like:

// define outside of this file e.g. somewhere in types
export const TASK_VIEWS = {
  LIST: 'list',
  GANTT: 'gantt',
  TABLE: 'table',
  KANBAN: 'kanban',
} as const

export type TaskView = typeof TASK_VIEWS[keyof typeof TASK_VIEWS]

I'm not so good in the use of TS enums yet but I have heard quite often hat using objects / arrays with as const and then getting the types from them can have advantages. E.g. if I remember corretly iteration is easier.

So as a result:

type SelectedViewMapper = Record<IList['id'], TaskView>

We can then also define the options like this:

export const TASK_VIEW_OPTION_MAP : Record<TaskView, string> = {
  list: t('list.list.title'),
  gantt: t('list.gantt.title'),
  table: t('list.table.title'),
  kanban: t('list.kanban.title'),
}
Use `Record` with `IList['id']`. I think in the future even if our ids are defined as numbers in the api it might make sense to cast them as strings in the frontend. I got the idea while reading the zod documentation. See: https://github.com/colinhacks/zod#record-key-type under **A note on numerical keys**. We should define the types of possible views only once. Something like: ```ts // define outside of this file e.g. somewhere in types export const TASK_VIEWS = { LIST: 'list', GANTT: 'gantt', TABLE: 'table', KANBAN: 'kanban', } as const export type TaskView = typeof TASK_VIEWS[keyof typeof TASK_VIEWS] ``` I'm not so good in the use of TS enums yet but I have heard quite often hat using objects / arrays with `as const` and then getting the types from them can have advantages. E.g. if I remember corretly iteration is easier. So as a result: ```ts type SelectedViewMapper = Record<IList['id'], TaskView> ``` We can then also define the options like this: ```ts export const TASK_VIEW_OPTION_MAP : Record<TaskView, string> = { list: t('list.list.title'), gantt: t('list.gantt.title'), table: t('list.table.title'), kanban: t('list.kanban.title'), } ```
konrad commented 2022-09-08 12:11:55 +00:00
Outdated
Review
Copy Link

Makes a lot of sense. I knew there had to be a way to do this properly :)

Makes a lot of sense. I knew there had to be a way to do this properly :)
konrad commented 2022-09-08 12:13:15 +00:00
Outdated
Review
Copy Link

I've called it ListView though because this is about lists and not directly about tasks.

I've called it `ListView` though because this is about lists and not directly about tasks.
👍 1
list: t('list.list.title'),
gantt: t('list.gantt.title'),
table: t('list.table.title'),
kanban: t('list.kanban.title'),
}))
const copy = useCopyToClipboard()
watch(
() => props.listId,
@ -225,7 +250,11 @@ async function load(listId: IList['id']) {
return
}
linkShares.value = await linkShareService.getAll({listId})
const links = await linkShareService.getAll({listId})
links.forEach((l: ILinkShare) => {
selectedView.value[l.id] = 'list'
})
linkShares.value = links
}
async function add(listId: IList['id']) {
@ -257,15 +286,15 @@ async function remove(listId: IList['id']) {
}
}
konrad marked this conversation as resolved Outdated
dpschen commented 2022-09-08 09:23:21 +00:00
Outdated
Review
Copy Link

Use then the TaskView type here.

Use then the `TaskView` type here.
konrad commented 2022-09-08 12:12:27 +00:00
Outdated
Review
Copy Link

Done.

Done.
function getShareLink(hash: string) {
return frontendUrl.value + 'share/' + hash + '/auth'
function getShareLink(hash: string, view: ListView = LIST_VIEWS.LIST) {
return frontendUrl.value + 'share/' + hash + '/auth?view=' + view
}
</script>
<style lang="scss" scoped>
// FIXME: I think this is not needed
.sharables-list:not(.card-content) {
overflow-y: auto
overflow-y: auto
}
@include modal-transition();

7
src/i18n/lang/en.json
View File

@ -242,7 +242,9 @@
"remove": "Remove a link share",
"removeText": "Are you sure you want to remove this link share? It will no longer be possible to access this list with this link share. This cannot be undone!",
"createSuccess": "The link share was successfully created.",
"deleteSuccess": "The link share was successfully deleted"
"deleteSuccess": "The link share was successfully deleted",
"view": "View",
"sharedBy": "Shared by {0}"
},
"userTeam": {
"typeUser": "user | users",
@ -264,9 +266,6 @@
},
"attributes": {
"link": "Link",
"name": "Name",
"sharedBy": "Shared by",
"right": "Right",
"delete": "Delete"
}
},

8
src/types/ListView.ts Normal file
View File

@ -0,0 +1,8 @@
export const LIST_VIEWS = {
LIST: 'list',
GANTT: 'gantt',
TABLE: 'table',
KANBAN: 'kanban',
} as const
export type ListView = typeof LIST_VIEWS[keyof typeof LIST_VIEWS]

8
src/views/sharing/LinkSharingAuth.vue
View File

@ -41,6 +41,7 @@ import {useTitle} from '@vueuse/core'
import Message from '@/components/misc/message.vue'
import {LOGO_VISIBLE} from '@/store/mutation-types'
import {LIST_VIEWS, type ListView} from '@/types/ListView'
const {t} = useI18n({useScope: 'global'})
useTitle(t('sharing.authenticating'))
@ -79,7 +80,12 @@ function useAuth() {
? route.query.logoVisible === 'true'
: true
store.commit(LOGO_VISIBLE, logoVisible)
router.push({name: 'list.list', params: {listId}})
const view = route.query.view && Object.values(LIST_VIEWS).includes(route.query.view as ListView)
? route.query.view
: 'list'
router.push({name: `list.${view}`, params: {listId}})
} catch (e: any) {
if (e.response?.data?.code === 13001) {
authenticateWithPassword.value = true