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 +7 -11
1 changed files with 7 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 26d02d5593 - Show all commits

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

@ -145,17 +145,11 @@
<td>
<div class="select">
<select v-model="selectedView[s.id]">
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
<option value="list">
{{ $t('list.list.title') }}
</option>
<option value="gantt">
{{ $t('list.gantt.title') }}
</option>
<option value="table">
{{ $t('list.table.title') }}
</option>
<option value="kanban">
{{ $t('list.kanban.title') }}
<option
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
v-for="v in availableViews"
:value="v"
:key="v">
{{ $t('list.' + v + '.title') }}
</option>
</select>
</div>
@ -234,6 +228,8 @@ type SelectedViewMapper = Record<IList['id'], ListView>
const selectedView = ref<SelectedViewMapper>({})
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
const availableViews = computed(() => Object.values(LIST_VIEWS))
const copy = useCopyToClipboard()
watch(
() => props.listId,