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

feat: improved types #2547

Merged
konrad merged 17 commits from dpschen/frontend:feature/improved-types into main 2022-11-02 16:06:57 +00:00
Conversation 7 Commits 17 Files Changed 63 +444 -340
dpschen commented 2022-10-16 17:48:47 +00:00
Member
Copy Link

This is a collection of unrelated type improvements that started growing while I was working on other stuff.

I don't know why my local pnpm always wants to upgrade the pnpm lockfile to 5.4 / why the dependabot lockfile always stays at 5.3. Any idea?

This is a collection of unrelated type improvements that started growing while I was working on other stuff. ---- I don't know why my local pnpm always wants to upgrade the pnpm lockfile to 5.4 / why the dependabot lockfile always stays at 5.3. Any idea?
dpschen added the
changes requested
area/internal-code
 labels 2022-10-16 17:48:47 +00:00
dpschen force-pushed feature/improved-types from 8ce5d0345c to 6888b19cf3 2022-10-16 23:51:00 +00:00 Compare
frederick commented 2022-10-16 23:55:35 +00:00
Member
Copy Link

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2547-feature-improved-types--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 dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://2547-feature-improved-types--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.
konrad commented 2022-10-17 12:19:27 +00:00
Owner
Copy Link

I don't know why my local pnpm always wants to upgrade the pnpm lockfile to 5.4 / why the dependabot lockfile always stays at 5.3. Any idea?

This might be caused by renovate using an outated pnpm version. I've just upgraded renovate so this should be fixed now.

> I don't know why my local pnpm always wants to upgrade the pnpm lockfile to 5.4 / why the dependabot lockfile always stays at 5.3. Any idea? This might be caused by renovate using an outated pnpm version. I've just upgraded renovate so this should be fixed now.
👍 1
dpschen force-pushed feature/improved-types from 6888b19cf3 to ba218180a7 2022-10-17 13:56:41 +00:00 Compare
dpschen force-pushed feature/improved-types from ba218180a7 to a4f24631a9 2022-10-17 14:06:10 +00:00 Compare
dpschen reviewed 2022-10-19 09:02:26 +00:00
src/components/base/BaseButton.vue
@ -1,1 +3,4 @@
<!-- just a normal link -->
<!-- a button it shall be -->
<!-- note that we only pass the click listener here -->
<template>
dpschen commented 2022-10-19 09:02:26 +00:00
Author
Member

I had to change the way how the BaseButton works for better type support.
It's a bit more repeatition but also very clear now what happens I guess.

Intersting sidenote: I originally placed the comments above inline to the switching elements but the vue renderer had some problems with that. Something about the component being a fragment which it should not be since it has just one root node. But seems like if you add a comment additionally to the root node (in case of a if / else that we have here), the comment counts as a node of the component. That makes sense, but is not intuitive, since you normally don't count a comment as content and prevents you from making good comments where they should be :)

I had to change the way how the BaseButton works for better type support. It's a bit more repeatition but also very clear now what happens I guess. Intersting sidenote: I originally placed the comments above inline to the switching elements but the vue renderer had some problems with that. Something about the component being a fragment which it should not be since it has just one root node. But seems like if you add a comment additionally to the root node (in case of a `if / else` that we have here), the comment counts as a node of the component. That makes sense, but is not intuitive, since you normally don't count a comment as content and prevents you from making good comments where they should be :)
👍 1
dpschen marked this conversation as resolved
dpschen reviewed 2022-10-19 09:02:58 +00:00
src/components/base/BaseButton.vue Outdated
@ -16,0 +46,4 @@
SUBMIT: 'submit',
} as const
export type BaseButtonTypes = typeof BASE_BUTTON_TYPES_MAP[keyof typeof BASE_BUTTON_TYPES_MAP] | undefined
dpschen commented 2022-10-19 09:02:58 +00:00
Author
Member

We export the types in order to use them in inheriting components

We export the types in order to use them in inheriting components
dpschen reviewed 2022-10-19 09:06:34 +00:00
src/components/base/BaseButton.vue Outdated
@ -53,2 +65,2 @@
rel?: string;
target?: string;
export interface BaseButtonProps extends HTMLAttributes {
type?: BaseButtonTypes
dpschen commented 2022-10-19 09:06:34 +00:00
Author
Member

As of now I had to add all possible combinations.
I tried to change the props based on the type but vue wouldn't accept that.
It boils down to that fact that vue currently doesn't support dynamic component types.

It might be that with the latest vue-tsc changes this is possible again.
Stricly speaking right now this component allows impossible prop combinations, these should be validated and filtered. But I hoped that I could spare that effort and solve it instead by establishing common knowledge on how this component is supposed to be used.

As of now I had to add all possible combinations. I tried to change the props [based on the type](https://www.typescriptlang.org/docs/handbook/2/conditional-types.html) but vue wouldn't accept that. It boils down to that fact that vue currently doesn't support dynamic component types. It might be that with the latest vue-tsc changes this is possible again. Stricly speaking right now this component allows impossible prop combinations, these should be validated and filtered. But I hoped that I could spare that effort and solve it instead by establishing common knowledge on how this component is supposed to be used.
konrad commented 2022-10-20 13:19:16 +00:00
Owner

I think that's fine. Invalid combinations will show up in PR reviews.

I think that's fine. Invalid combinations will show up in PR reviews.
👍 1
dpschen marked this conversation as resolved
dpschen changed title from WIP: feat: improved types to feat: improved types 2022-10-20 09:11:25 +00:00
dpschen requested review from konrad 2022-10-20 09:11:30 +00:00
konrad was assigned by dpschen 2022-10-20 09:11:33 +00:00
dpschen reviewed 2022-10-20 09:13:04 +00:00
src/components/input/button.vue Outdated
@ -65,2 +46,2 @@
const variantClass = computed(() => BUTTON_TYPES_MAP[props.variant])
// extending the props of the BaseButton
export interface ButtonProps extends BaseButtonProps {
dpschen commented 2022-10-20 09:13:04 +00:00
Author
Member

By extending the BaseButtonProps we inherit them and get proper autocomplete! :)

By extending the BaseButtonProps we inherit them and get proper autocomplete! :)
🎉 1
dpschen marked this conversation as resolved
dpschen reviewed 2022-10-20 09:14:35 +00:00
src/components/misc/dropdown-item.vue Outdated
@ -4,3 +2,1 @@
v-bind="elementBindings"
:to="to"
class="dropdown-item">
<BaseButton class="dropdown-item">
dpschen commented 2022-10-20 09:14:35 +00:00
Author
Member

By reusing the BaseButton the dropdown-item got much simpler :)

By reusing the BaseButton the dropdown-item got much simpler :)
🎉 1
konrad commented 2022-10-20 13:23:07 +00:00
Owner

Wow it really looks a lot simpler now.

Wow it really looks a lot simpler now.
dpschen marked this conversation as resolved
dpschen reviewed 2022-10-20 09:15:14 +00:00
src/components/misc/dropdown.vue Outdated
@ -25,3 +26,3 @@
defineProps({
triggerIcon: {
type: String,
type: String as PropType<IconProp>,
dpschen commented 2022-10-20 09:15:14 +00:00
Author
Member

Using IconProp enables autocomplete for the icons props

Using `IconProp` enables autocomplete for the icons props
👍 1
dpschen marked this conversation as resolved
dpschen reviewed 2022-10-20 09:16:13 +00:00
src/components/misc/subscription.vue Outdated
@ -14,3 +14,3 @@
v-tooltip="tooltipText"
@click="changeSubscription"
:class="{'is-disabled': disabled}"
:disabled="disabled"
dpschen commented 2022-10-20 09:16:13 +00:00
Author
Member

Since dropdown inherits from basebutton it automatically inherits props like disabled

Since dropdown inherits from basebutton it automatically inherits props like disabled
dpschen marked this conversation as resolved
dpschen reviewed 2022-10-20 09:20:27 +00:00
src/helpers/parseDateOrNull.ts Outdated
@ -7,3 +7,3 @@
}
if ((typeof date === 'string' || date instanceof String) && !date.startsWith('0001')) {
if ((typeof date === 'string') && !date.startsWith('0001')) {
dpschen commented 2022-10-20 09:20:27 +00:00
Author
Member

We should stop using String(). Regardless date strings normally come from toISOString() which produces a type string

We should stop using `String()`. Regardless date strings normally come from `toISOString()` which produces a type string
konrad commented 2022-10-20 13:25:54 +00:00
Owner

There might be cases though where a date string comes from the api and needs to be parsed here. (or at leat that was the reason why I added this function originally)

There might be cases though where a date string comes from the api and needs to be parsed here. (or at leat that was the reason why I added this function originally)
dpschen commented 2022-10-27 13:56:53 +00:00
Author
Member

Unsure if we talk here about the same. I'm talking about the general use of String() to convert something to a string. Afaik we don't do that anywhere for the data that's coming from the api. We do use the parseDateOrNull function but it should would without the instanceof String. If you think it should stay, I'm happy to revert this change.

Unsure if we talk here about the same. I'm talking about the general use of `String()` to convert something to a string. Afaik we don't do that anywhere for the data that's coming from the api. We do use the `parseDateOrNull` function but it should would without the `instanceof String`. If you think it should stay, I'm happy to revert this change.
👍 1
dpschen reviewed 2022-10-20 09:21:10 +00:00
src/modelTypes/IUser.ts Outdated
@ -21,2 +21,4 @@
updated: Date
settings: IUserSettings
isLocalUser: boolean
dpschen commented 2022-10-20 09:21:10 +00:00
Author
Member

Is it correct to add these properties? They seemed to be missing.

Is it correct to add these properties? They seemed to be missing.
konrad commented 2022-10-20 13:27:17 +00:00
Owner

yes, that's correct.

yes, that's correct.
👍 1
dpschen marked this conversation as resolved
dpschen reviewed 2022-10-20 09:22:20 +00:00
src/models/user.ts Outdated
@ -28,6 +28,9 @@ export default class UserModel extends AbstractModel<IUser> implements IUser {
updated: Date
settings: IUserSettings
isLocalUser: boolean // FIXME: what should this be
dpschen commented 2022-10-20 09:22:20 +00:00
Author
Member

I assumed that the type here is boolean.

I assumed that the type here is boolean.
konrad commented 2022-10-20 13:27:40 +00:00
Owner

That's correct, same as in the interface.

That's correct, same as in the interface.
👍 1
dpschen commented 2022-10-27 13:57:54 +00:00
Author
Member

I removed the comment.

I removed the comment.
👍 1
konrad marked this conversation as resolved
dpschen reviewed 2022-10-20 09:29:35 +00:00
tsconfig.app.json Outdated
@ -19,3 +19,3 @@
},
"vueCompilerOptions": {
"strictTemplates": true
// "strictTemplates": true
dpschen commented 2022-10-20 09:29:35 +00:00
Author
Member

Unsure about this

Unsure about this
dpschen commented 2022-10-27 13:59:00 +00:00
Author
Member

Biggest reason why I can't judge this: Even after the merge of this branch we still have so many type errors that I don't know which errors appear / disappear when I toggle this option.

Biggest reason why I can't judge this: Even after the merge of this branch we still have so many type errors that I don't know which errors appear / disappear when I toggle this option.
konrad commented 2022-10-27 19:40:37 +00:00
Owner

Is there any difference in the number of errors?

Is there any difference in the number of errors?
dpschen commented 2022-10-28 16:25:27 +00:00
Author
Member

Without "strictTemplates": true (I guess the default is false ;) )

Found 917 errors in 154 files.

With "strictTemplates": true

Found 949 errors in 159 files.

As said: I do not know which errors these are. It might also be that some disappear while others emerge.

Without `"strictTemplates": true` (I guess the default is false ;) ) ``` Found 917 errors in 154 files. ``` With `"strictTemplates": true` ``` Found 949 errors in 159 files. ``` As said: I do not know which errors these are. It might also be that some disappear while others emerge.
konrad reviewed 2022-10-20 13:30:11 +00:00
src/components/base/BaseButton.vue Outdated
@ -2,2 +7,2 @@
<component
:is="componentNodeName"
<div
v-if="disabled === true && (to !== undefined || href !== undefined)"
konrad commented 2022-10-20 13:17:08 +00:00
Owner

Are there any cases where it makes sense for this to be a div?

Are there any cases where it makes sense for this to be a div?
dpschen commented 2022-10-27 13:59:49 +00:00
Author
Member

Yes.
Out of a semantic perspective a disabled button / link is not a button / link.

Yes. Out of a semantic perspective a disabled button / link is not a button / link.
src/components/base/BaseButton.vue Outdated
@ -7,0 +24,4 @@
class="base-button"
rel="noreferrer noopener nofollow"
target="_blank"
ref="button"
konrad commented 2022-10-20 13:17:43 +00:00
Owner

This looks like it's missing the href attribute.

This looks like it's missing the `href` attribute.
👍 1
dpschen commented 2022-10-27 14:01:04 +00:00
Author
Member

Good catch! Fixed

Good catch! Fixed
dpschen marked this conversation as resolved
src/components/misc/popup.vue Outdated
@ -16,2 +16,2 @@
open.value = !open.value
}
import {ref} from 'vue'
import {onClickOutside} from '@vueuse/core'
konrad commented 2022-10-20 13:24:03 +00:00
Owner

I wonder if we even need the helper function I built back in the day?

I wonder if we even need the helper function I built back in the day?
👍 1
dpschen commented 2022-10-27 14:02:28 +00:00
Author
Member

I already try to remove it piece by piece :)

I already try to remove it piece by piece :)
👍 1
dpschen marked this conversation as resolved
src/types/vue-fontawesome.ts Outdated
@ -0,0 +1,40 @@
// copied and slightly modified from unmerged pull request that corrects types
// https://github.com/FortAwesome/vue-fontawesome/pull/355
konrad commented 2022-10-20 13:29:22 +00:00
Owner

We need a reminder to remove this once the PR is merged (is that even possible entirely?)

We need a reminder to remove this once the PR is merged (is that even possible entirely?)
👍 1
dpschen commented 2022-10-27 14:03:36 +00:00
Author
Member

I subscribed to the thread of that PR. So I should get a notification if there are any changes.

I subscribed to the thread of that PR. So I should get a notification if there are any changes.
👍 1
konrad marked this conversation as resolved
dpschen commented 2022-10-27 14:04:06 +00:00
Author
Member
Copy Link

Anything blocking this?

Anything blocking this?
konrad commented 2022-10-27 19:41:50 +00:00
Owner
Copy Link

Anything blocking this?

Now that the gantt PR is merged there are conflicts :)

I'll do another review but I think we're good.

> Anything blocking this? Now that the gantt PR is merged there are conflicts :) I'll do another review but I think we're good.
konrad commented 2022-10-27 19:45:01 +00:00
Owner
Copy Link

Looks like the link button does not work :/

(For example when migrating from external services)

Looks like the link button does not work :/ (For example when migrating from external services)
dpschen force-pushed feature/improved-types from d2c153788b to 7f1100e819 2022-10-28 14:46:29 +00:00 Compare
dpschen commented 2022-10-28 16:50:37 +00:00
Author
Member
Copy Link

Looks like the link button does not work :/

(For example when migrating from external services)

I think this was beacause of the missing href. Can't reproduce anymore, so should be solved.

> Looks like the link button does not work :/ > > (For example when migrating from external services) I think this was beacause of the missing `href`. Can't reproduce anymore, so should be solved.
dpschen force-pushed feature/improved-types from 87511139fe to 781dcb30d6 2022-10-31 18:24:44 +00:00 Compare
dpschen commented 2022-10-31 18:25:06 +00:00
Author
Member
Copy Link

Can you check again?

Can you check again?
dpschen force-pushed feature/improved-types from 781dcb30d6 to 480aa8813e 2022-10-31 21:42:44 +00:00 Compare
dpschen added 2 commits 2022-11-01 12:13:07 +00:00
continuous-integration/drone/pr Build is passing Details
3c5bfcc6f3
fix: potential issue with refs in Avatar
dpschen added 1 commit 2022-11-01 13:27:48 +00:00
continuous-integration/drone/pr Build is passing Details
e01df4d369
fix: coverImageAttachmentId
konrad approved these changes 2022-11-02 16:06:18 +00:00
konrad merged commit 0ff0d8c5b8 into main 2022-11-02 16:06:57 +00:00
konrad referenced this issue from a commit 2022-11-02 16:06:57 +00:00
feat: improved types (#2547)
konrad deleted branch feature/improved-types 2022-11-02 16:06:57 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
konrad
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
konrad
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#2547
No description provided.
Delete Branch "dpschen/frontend:feature/improved-types"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?