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
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 2022-09-07 20:40:11 +00:00
Member

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 2022-09-08 09:49:37 +00:00
@ -83,3 +83,1 @@
<th>{{ $t('list.share.attributes.name') }}</th>
<th>{{ $t('list.share.attributes.sharedBy') }}</th>
<th>{{ $t('list.share.attributes.right') }}</th>
<th>&nbsp;</th>

Why is the &nbsp; necessary?

Why is the `&nbsp;` necessary?
Author
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
@ -207,6 +227,12 @@ const showDeleteModal = ref(false)
const linkIdToDelete = ref(0)
const showNewForm = ref(false)
interface SelectedViewMapper {

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'), } ```
Author
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 :)
Author
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
@ -257,3 +287,2 @@
function getShareLink(hash: string) {
return frontendUrl.value + 'share/' + hash + '/auth'
function getShareLink(hash: string, view: string = 'list') {

Use then the TaskView type here.

Use then the `TaskView` type here.
Author
Owner

Done.

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

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 ```
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
Member

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 2022-09-08 11:57:06 +00:00 Compare
konrad added 1 commit 2022-09-08 11:58:59 +00:00
continuous-integration/drone/pr Build is passing Details
d91d1fecf1
chore: remove &nbsp;
konrad added 1 commit 2022-09-08 12:11:26 +00:00
Author
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 2022-09-08 12:21:29 +00:00
@ -140,2 +146,2 @@
{{ $t('list.share.right.read') }}
</template>
<div class="select">
<select v-model="selectedView[s.id]" id="linkShareView">

The id seems unused

The id seems unused
Author
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 2022-09-08 12:22:31 +00:00
@ -141,1 +146,3 @@
</template>
<div class="select">
<select v-model="selectedView[s.id]" id="linkShareView">
<option value="list">

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

Done.

Done.

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

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

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 2022-09-08 14:27:00 +00:00
continuous-integration/drone/pr Build is passing Details
5f678e2449
chore: remove unused id
konrad added 1 commit 2022-09-08 14:29:33 +00:00
continuous-integration/drone/pr Build is passing Details
26d02d5593
feat: programmatically generate list of available views
konrad added 1 commit 2022-09-13 20:04:29 +00:00
continuous-integration/drone/pr Build is failing Details
e67fc7fb7e
fix: use proper computed for available views list
konrad added 1 commit 2022-09-13 20:07:55 +00:00
continuous-integration/drone/pr Build is passing Details
2b82df5dbd
Merge branch 'main' into feature/redirect-to-specific-view
# Conflicts:
#	src/components/sharing/linkSharing.vue
Member

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;`
Member

Related: #2361

Related: https://kolaente.dev/vikunja/frontend/issues/2361
konrad was assigned by dpschen 2022-09-14 09:38:07 +00:00
konrad added 1 commit 2022-09-14 14:17:37 +00:00
Author
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 2022-09-14 14:28:03 +00:00
continuous-integration/drone/pr Build was killed Details
224cea33ce
feat: make share link name italic
konrad added 1 commit 2022-09-14 14:31:10 +00:00
continuous-integration/drone/pr Build is passing Details
6576b6148c
feat: move the url link to the bottom of the items
Author
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 2022-09-14 15:53:41 +00:00
Member

Moved to new issue instead :)

Moved [to new issue](https://kolaente.dev/vikunja/frontend/issues/2362) instead :)
konrad merged commit a6e9b36bd6 into main 2022-09-14 16:37:56 +00:00
konrad deleted branch feature/redirect-to-specific-view 2022-09-14 16:37:56 +00:00
First-time contributor

Thank you for implementing this! New Release? :)

Thank you for implementing this! New Release? :)
Author
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.
This repo is archived. You cannot comment on pull requests.
No description provided.