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 3 months ago
Owner

This PR introduces the view query parameter to be appended at the end of any link share. If provided, the link share will automatically switch to the view passed in the query, assuming it is one of list, gantt, table or kanban.

It also cleans up the link share table a bit as it became too crowded.

Before:

image

After:

image

This PR introduces the `view` query parameter to be appended at the end of any link share. If provided, the link share will automatically switch to the view passed in the query, assuming it is one of `list`, `gantt`, `table` or `kanban`. It also cleans up the link share table a bit as it became too crowded. Before: ![image](/attachments/247d88fd-403d-47b8-88b6-79f47be21263) After: ![image](/attachments/19ac2fd3-ca30-4173-a600-a09e6f649ed4)
konrad requested review from dpschen 3 months ago
Collaborator

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2335-feature-redirect-to-specific-vie--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://2335-feature-redirect-to-specific-vie--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen requested changes 3 months ago
<th>{{ $t('list.share.attributes.name') }}</th>
<th>{{ $t('list.share.attributes.sharedBy') }}</th>
<th>{{ $t('list.share.attributes.right') }}</th>
<th>&nbsp;</th>
Collaborator

Why is the &nbsp; necessary?

Why is the `&nbsp;` necessary?
Poster
Owner

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

I thought it was necessary, looks like it isn't.
konrad marked this conversation as resolved
const linkIdToDelete = ref(0)
const showNewForm = ref(false)
interface SelectedViewMapper {
Collaborator

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'), } ```
Poster
Owner

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 :)
Poster
Owner

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.
konrad marked this conversation as resolved
function getShareLink(hash: string) {
return frontendUrl.value + 'share/' + hash + '/auth'
function getShareLink(hash: string, view: string = 'list') {
Collaborator

Use then the TaskView type here.

Use then the `TaskView` type here.
Poster
Owner

Done.

Done.
konrad marked this conversation as resolved
password: password.value,
})
router.push({name: 'list.list', params: {listId}})
let view = 'list'
Collaborator

Use the TaskView type.
We can then check with:

view = route.query.view && TASK_VIEWS.includes(route.query.view)
  ? route.query.view
  : TASK_VIEW_DEFAULT
Use the `TaskView` type. We can then check with: ```ts view = route.query.view && TASK_VIEWS.includes(route.query.view) ? route.query.view : TASK_VIEW_DEFAULT ```
Poster
Owner

Done.

Done.
konrad marked this conversation as resolved
Collaborator

Where did the 'Name' column go?

I recently shared some assets via Nextcloud and was really happy when I could share the same folder with different links and create a name so that I am able to separate them.

Sideidea:
Maybe it would be cool we should even define a stupid default name.
Something like this onky vijunja-flavoured https://github.com/Atrox/haikunatorjs

Where did the 'Name' column go? I recently shared some assets via Nextcloud and was really happy when I could share the same folder with different links and create a name so that I am able to separate them. Sideidea: Maybe it would be cool we should even define a stupid default name. Something like this onky vijunja-flavoured https://github.com/Atrox/haikunatorjs
konrad force-pushed feature/redirect-to-specific-view from 8831c83261 to 7a457eb161 3 months ago
konrad added 1 commit 3 months ago
d91d1fecf1
chore: remove &nbsp;
konrad added 1 commit 3 months ago
Poster
Owner

Where did the 'Name' column go?

The column is gone, but if the link has a name it will still be shown. It then looks like this:

image

The shre in my screenshots didn't have a name so nothing showed up.

> Where did the 'Name' column go? The column is gone, but if the link has a name it will still be shown. It then looks like this: ![image](/attachments/82abd7b5-cff6-40cf-a609-42981056e230) The shre in my screenshots didn't have a name so nothing showed up.
dpschen reviewed 3 months ago
{{ $t('list.share.right.read') }}
</template>
<div class="select">
<select v-model="selectedView[s.id]" id="linkShareView">
Collaborator

The id seems unused

The id seems unused
Poster
Owner

Removed it. Was a leftover from an earlier version.

Removed it. Was a leftover from an earlier version.
konrad marked this conversation as resolved
dpschen reviewed 3 months ago
</template>
<div class="select">
<select v-model="selectedView[s.id]" id="linkShareView">
<option value="list">
Collaborator

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.
Poster
Owner

Done.

Done.
Collaborator

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

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
Poster
Owner

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?
Collaborator

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.
Poster
Owner

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.
Collaborator

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
konrad marked this conversation as resolved
konrad added 1 commit 3 months ago
5f678e2449
chore: remove unused id
konrad added 1 commit 3 months ago
konrad added 1 commit 3 months ago
e67fc7fb7e
fix: use proper computed for available views list
konrad added 1 commit 3 months ago
2b82df5dbd
Merge branch 'main' into feature/redirect-to-specific-view
Collaborator

EDIT:
I created a new issue with these tasks

When testing the sharing functionality again I stumbled over a few other points:

  • Unsure: move the 'Create a new link share'-Button and the create-form below the list of already created shares. This way it's clear that the newly created share is added to the bottom of the list. Possible downside: out of view on mobile and small desktop windows.

  • Non reversible UI action: Clicking the 'Create a new link share'-Button should be reversible if you clicked there by mistake. Aka: add a 'Cancel' or 'x' button in the inline form.

  • Inline form the create a new share link is hard to tell apart from the rest of the form.
    image

  • Missing space between 'Cancel' and 'Create' button. This might be a sideeffect from some of my Modal changes, but might make sense to look into this here. Seems like a gap property is missing for .card-footer.
    image

  •  When clicking the copy button it should be indicated that the new link is now in the clipboard. E.g. marking the copy button green and adding checkmark or success message. Would prefer the checkmark since this way the user doesn't loose context and the feedback is immediately and inline. After a few seconds the button should go back in its initial state. This button or maybe the whole input should be abstracted dedicated component

  •  Long share names don't look good. Especially the input form gets too small. It seems fine for me that you can only see the link partly but only showing https seems to be not enough. Maybe we should put the share name in a new line above the rest of the info, since we never know its length.
    image

  • This is a general issue on mobile: It's hard to make the tooltip of 'What is a share link?' appear. Tapping makes it appear but it will directly disappear again. Since the info is so short I think we can remove the tooltip here completely and simply show the explaining text.

  • Picky: The tap area of the copy button seems to small on mobile. It would benefit from an aspect-ratio: 1/1;

**EDIT:** I created [a new issue](https://kolaente.dev/vikunja/frontend/issues/2362) with these tasks When testing the sharing functionality again I stumbled over a few other points: - [ ] Unsure: move the 'Create a new link share'-Button and the create-form below the list of already created shares. This way it's clear that the newly created share is added to the bottom of the list. Possible downside: out of view on mobile and small desktop windows. - [ ] Non reversible UI action: Clicking the 'Create a new link share'-Button should be reversible if you clicked there by mistake. Aka: add a 'Cancel' or 'x' button in the inline form. - [ ] Inline form the create a new share link is hard to tell apart from the rest of the form. ![image](/attachments/1b4303a8-e358-4454-a9a3-64813374d640) - [ ] Missing space between 'Cancel' and 'Create' button. This might be a sideeffect from some of my Modal changes, but might make sense to look into this here. Seems like a `gap` property is missing for `.card-footer`. ![image](/attachments/98923d63-ee66-47dc-873d-54aaab054a30) - [ ] When clicking the copy button it should be indicated that the new link is now in the clipboard. E.g. marking the copy button green and adding checkmark or success message. Would prefer the checkmark since this way the user doesn't loose context and the feedback is immediately and inline. After a few seconds the button should go back in its initial state. This button or maybe the whole input should be abstracted dedicated component - [x] Long share names don't look good. Especially the input form gets too small. It seems fine for me that you can only see the link partly but only showing `https` seems to be not enough. Maybe we should put the share name in a new line above the rest of the info, since we never know its length. ![image](/attachments/c8020af2-2448-4f39-b02b-7b38a6bd64fd) - [ ] This is a general issue on mobile: It's hard to make the tooltip of 'What is a share link?' appear. Tapping makes it appear but it will directly disappear again. Since the info is so short I think we can remove the tooltip here completely and simply show the explaining text. - [ ] Picky: The tap area of the copy button seems to small on mobile. It would benefit from an `aspect-ratio: 1/1;`
Collaborator

Related: #2361

Related: https://kolaente.dev/vikunja/frontend/issues/2361
konrad was assigned by dpschen 3 months ago
konrad added 1 commit 3 months ago
Poster
Owner

Those are all good points! However as they are not all related to the changes in this PR I think we should do a follow-up PR with them in order to keep this PR small and reviewable. Maybe in combination with #2361.

IHMO only the 6th point on the list (long names) is something related to the changes I made here. The other ones are problems in the current dev release as well.

Those are all good points! However as they are not all related to the changes in this PR I think we should do a follow-up PR with them in order to keep this PR small and reviewable. Maybe in combination with https://kolaente.dev/vikunja/frontend/issues/2361. IHMO only the 6th point on the list (long names) is something related to the changes I made here. The other ones are problems in the current dev release as well.
konrad added 1 commit 3 months ago
224cea33ce
feat: make share link name italic
konrad added 1 commit 3 months ago
6576b6148c
feat: move the url link to the bottom of the items
Poster
Owner

My quick and easy fix for the problem with too long links is this:

image

I also made the name italic so that it is easier to differentiate from the other info.

My quick and easy fix for the problem with too long links is this: ![image](/attachments/dd90c44b-914c-4f5a-bae8-fdf50be37c28) I also made the name italic so that it is easier to differentiate from the other info.
dpschen approved these changes 3 months ago
Collaborator

Moved to new issue instead :)

Moved [to new issue](https://kolaente.dev/vikunja/frontend/issues/2362) instead :)
konrad merged commit a6e9b36bd6 into main 3 months ago
konrad deleted branch feature/redirect-to-specific-view 3 months ago

Thank you for implementing this! New Release? :)

Thank you for implementing this! New Release? :)
Poster
Owner

Thank you for implementing this! New Release? :)

There's a few more things I'd like to bundle up for the next release.

> Thank you for implementing this! New Release? :) There's a few more things I'd like to bundle up for the next release.

Reviewers

dpschen approved these changes 3 months ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as a6e9b36bd6.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#2335
Loading…
There is no content yet.