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

127
src/components/base/BaseButton.vue
View File

@ -1,18 +1,52 @@
<!-- a disabled link of any kind is not a link -->
<!-- we have a router link -->
<!-- just a normal link -->
<!-- a button it shall be -->
<!-- note that we only pass the click listener here -->
<template>
dpschen marked this conversation as resolved
dpschen commented 2022-10-19 09:02:26 +00:00
Review
Copy Link

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
<component
:is="componentNodeName"
<div
v-if="disabled === true && (to !== undefined || href !== undefined)"
konrad commented 2022-10-20 13:17:08 +00:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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.
class="base-button"
:class="{ 'base-button--type-button': isButton }"
v-bind="elementBindings"
:disabled="disabled || undefined"
:aria-disabled="disabled || undefined"
ref="button"
>
<slot/>
</component>
</div>
<router-link
v-else-if="to !== undefined"
:to="to"
class="base-button"
ref="button"
>
<slot/>
</router-link>
<a v-else-if="href !== undefined"
class="base-button"
rel="noreferrer noopener nofollow"
target="_blank"
ref="button"
dpschen marked this conversation as resolved Outdated
konrad commented 2022-10-20 13:17:43 +00:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

Good catch! Fixed

Good catch! Fixed
>
<slot/>
</a>
<button
v-else
:type="type"
class="base-button base-button--type-button"
:disabled="disabled || undefined"
ref="button"
@click="(event: MouseEvent) => emit('click', event)"
>
<slot/>
</button>
</template>
<script lang="ts">
export default { inheritAttrs: false }
const BASE_BUTTON_TYPES_MAP = {
BUTTON: 'button',
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
Outdated
Review
Copy Link

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

We export the types in order to use them in inheriting components
</script>
<script lang="ts" setup>
@ -20,77 +54,36 @@ export default { inheritAttrs: false }
// by doing so we make it easy abstract the functionality from style and enable easier and semantic
// correct button and link usage. Also see: https://css-tricks.com/a-complete-guide-to-links-and-buttons/#accessibility-considerations
// the component tries to heuristically determine what it should be checking the props (see the
// componentNodeName and elementBindings ref for this).
// the component tries to heuristically determine what it should be checking the props
// NOTE: Do NOT use buttons with @click to push routes. => Use router-links instead!
import { ref, watchEffect, computed, useAttrs, type PropType } from 'vue'
import {unrefElement} from '@vueuse/core'
import {ref, type HTMLAttributes} from 'vue'
import type {RouteLocationNamedRaw} from 'vue-router'
const BASE_BUTTON_TYPES_MAP = Object.freeze({
button: 'button',
submit: 'submit',
})
type BaseButtonTypes = keyof typeof BASE_BUTTON_TYPES_MAP
const props = defineProps({
type: {
type: String as PropType<BaseButtonTypes>,
default: 'button',
},
disabled: {
type: Boolean,
default: false,
},
})
const componentNodeName = ref<Node['nodeName']>('button')
interface ElementBindings {
type?: string;
rel?: string;
target?: string;
export interface BaseButtonProps extends HTMLAttributes {
type?: BaseButtonTypes
dpschen marked this conversation as resolved Outdated
dpschen commented 2022-10-19 09:06:34 +00:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
disabled?: boolean
to?: RouteLocationNamedRaw
href?: string
}
const elementBindings = ref({})
export interface BaseButtonEmits {
(e: 'click', payload: MouseEvent): void
}
const attrs = useAttrs()
watchEffect(() => {
// by default this component is a button element with the attribute of the type "button" (default prop value)
let nodeName = 'button'
let bindings: ElementBindings = {type: props.type}
const {
type = BASE_BUTTON_TYPES_MAP.BUTTON,
disabled = false,
} = defineProps<BaseButtonProps>()
// if we find a "to" prop we set it as router-link
if ('to' in attrs) {
nodeName = 'router-link'
bindings = {}
}
const emit = defineEmits<BaseButtonEmits>()
// if there is a href we assume the user wants an external link via a link element
// we also set a predefined value for the attribute rel, but make it possible to overwrite this by the user.
if ('href' in attrs) {
nodeName = 'a'
bindings = {
rel: 'noreferrer noopener nofollow',
target: '_blank',
}
}
componentNodeName.value = nodeName
elementBindings.value = {
...bindings,
...attrs,
}
})
const isButton = computed(() => componentNodeName.value === 'button')
const button = ref()
const button = ref<HTMLElement | null>(null)
function focus() {
button.value.focus()
unrefElement(button)?.focus()
}
defineExpose({