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

feat: Edit relative reminders #3248

Merged
konrad merged 27 commits from ce72/frontend:main into main 2023-06-10 17:04:10 +00:00
Conversation 18 Commits 27 Files Changed 18 +42 -48
4 changed files with 42 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit fb14eca634 - Show all commits

24
src/components/tasks/partials/reminder-detail.vue
View File

@ -1,8 +1,9 @@
<template>
<div>
<ReminderPeriod v-if="showRelativeReminder()" v-model="reminder" :disabled="disabled" @update:modelValue="() => updateData()"></ReminderPeriod>
<ReminderPeriod v-if="showRelativeReminder" v-model="reminder" :disabled="disabled"
dpschen marked this conversation as resolved Outdated
konrad commented 2023-03-28 09:57:47 +00:00
Outdated
Review
Copy Link

Nitpicking, but please format this like this:

<ReminderPeriod 
    v-if="showRelativeReminder"
    v-model="reminder"
    :disabled="disabled"
    @update:modelValue="() => updateData()"
/>

That way it's more readable.

Nitpicking, but please format this like this: ``` <ReminderPeriod v-if="showRelativeReminder" v-model="reminder" :disabled="disabled" @update:modelValue="() => updateData()" /> ``` That way it's more readable.
dpschen commented 2023-03-30 10:58:37 +00:00
Outdated
Review
Copy Link

Fixed in my changes, see #3248 (comment)

Fixed in my changes, see https://kolaente.dev/vikunja/frontend/pulls/3248#issuecomment-47907
@update:modelValue="() => updateData()"></ReminderPeriod>
<Datepicker
v-if="showAbsoluteReminder()"
v-if="showAbsoluteReminder"
v-model="reminderDate"
:disabled="disabled"
@close-on-change="() => setReminderDate()"
@ -14,8 +15,8 @@
import Datepicker from '@/components/input/datepicker.vue'
import ReminderPeriod from '@/components/tasks/partials/reminder-period.vue'
import TaskReminderModel from '@/models/taskReminder'
import type { ITaskReminder } from '@/modelTypes/ITaskReminder'
import { ref, watch, type PropType } from 'vue'
import type {ITaskReminder} from '@/modelTypes/ITaskReminder'
import {computed, ref, watch, type PropType} from 'vue'
const props = defineProps({
modelValue: {
@ -32,9 +33,12 @@ const emit = defineEmits(['update:modelValue'])
const reminder = ref<ITaskReminder>()
const reminderDate = ref()
const showAbsoluteReminder = computed(() => !reminder.value || !reminder.value?.relativeTo)
const showRelativeReminder = computed(() => !reminder.value || reminder.value?.relativeTo)
watch(
() => props.modelValue,
(value) => {
() => props.modelValue,
(value) => {
reminder.value = value
if (reminder.value && reminder.value.reminder) {
reminderDate.value = new Date(reminder.value.reminder)
@ -47,14 +51,6 @@ function updateData() {
emit('update:modelValue', reminder.value)
}
function showAbsoluteReminder() {
return !reminder.value || !reminder.value?.relativeTo
}
function showRelativeReminder() {
return !reminder.value || reminder.value?.relativeTo
}
function setReminderDate() {
if (!reminderDate.value) {
dpschen marked this conversation as resolved Outdated
konrad commented 2023-03-16 19:08:53 +00:00
Outdated
Review
Copy Link

Please use a computed for both show... functions instead.

Please use a `computed` for both `show...` functions instead.
ce72 commented 2023-03-17 09:35:55 +00:00
Outdated
Review
Copy Link

ok

ok
return

57
src/components/tasks/partials/reminder-period.vue
View File

@ -1,7 +1,7 @@
<template>
<div class="datepicker">
<BaseButton :disabled="disabled" class="show" v-if="!!reminder?.relativeTo" @click.stop="togglePeriodPopup">
{{ formatDuration(reminder.relativePeriod) }} <span v-html="formatBeforeAfter(reminder.relativePeriod)"></span>
{{ formatDuration(reminder.relativePeriod) }} {{ reminder.relativePeriod <= 0 ? '&le;' : '&gt;' }}
ce72 marked this conversation as resolved Outdated
konrad commented 2023-03-16 19:13:24 +00:00
Outdated
Review
Copy Link

What about inlining formatBeforeAfter to something like {{ reminder.relativePeriod <= 0 ? '&le;' : '&gt;' }}

What about inlining `formatBeforeAfter` to something like `{{ reminder.relativePeriod <= 0 ? '&le;' : '&gt;' }}`
ce72 commented 2023-03-17 09:36:03 +00:00
Outdated
Review
Copy Link

ok

ok
{{ formatRelativeTo(reminder.relativeTo) }}
</BaseButton>
<CustomTransition name="fade">
@ -10,15 +10,15 @@
<input
:disabled="disabled"
class="input"
placeholder="d"
:placeholder="$t('task.reminder.daysShort')"
v-model="periodInput.duration.days"
type="number"
min="0"
/> d
/> {{ $t('task.reminder.daysShort') }}
ce72 marked this conversation as resolved Outdated
konrad commented 2023-03-16 19:09:27 +00:00
Outdated
Review
Copy Link

Please add a translation string for the d. (It does not look like much, but still)

Please add a translation string for the `d`. (It does not look like much, but still)
konrad commented 2023-03-16 19:09:48 +00:00
Outdated
Review
Copy Link

Same for the placeholders.

Same for the placeholders.
ce72 commented 2023-03-17 09:36:12 +00:00
Outdated
Review
Copy Link

ok

ok
dpschen commented 2023-03-20 16:35:18 +00:00
Outdated
Review
Copy Link

To be honest I was confused by the d when I first saw the interface.
Why don't we write it out?

To be honest I was confused by the `d` when I first saw the interface. Why don't we write it out?
ce72 commented 2023-03-20 20:29:34 +00:00
Outdated
Review
Copy Link

ok

ok
<input
:disabled="disabled"
class="input"
placeholder="HH"
:placeholder="$t('task.reminder.hoursShort')"
v-model="periodInput.duration.hours"
type="number"
min="0"
@ -26,7 +26,7 @@
<input
:disabled="disabled"
class="input"
placeholder="MM"
:placeholder="$t('task.reminder.minutesShort')"
v-model="periodInput.duration.minutes"
type="number"
min="0"
@ -37,17 +37,15 @@
<option value="1">&gt;</option>
</select>
</div>
<div class="control">
<div class="select">
<select :disabled="disabled" v-model="periodInput.relativeTo" id="relativeTo">
<option :value="REMINDER_PERIOD_RELATIVE_TO_TYPES.DUEDATE">{{ $t('task.attributes.dueDate') }}</option>
<option :value="REMINDER_PERIOD_RELATIVE_TO_TYPES.STARTDATE">{{
$t('task.attributes.startDate')
}}
</option>
<option :value="REMINDER_PERIOD_RELATIVE_TO_TYPES.ENDDATE">{{ $t('task.attributes.endDate') }}</option>
</select>
</div>
<div class="select">
<select :disabled="disabled" v-model="periodInput.relativeTo" id="relativeTo">
<option :value="REMINDER_PERIOD_RELATIVE_TO_TYPES.DUEDATE">{{ $t('task.attributes.dueDate') }}</option>
<option :value="REMINDER_PERIOD_RELATIVE_TO_TYPES.STARTDATE">{{
$t('task.attributes.startDate')
}}
</option>
<option :value="REMINDER_PERIOD_RELATIVE_TO_TYPES.ENDDATE">{{ $t('task.attributes.endDate') }}</option>
</select>
</div>
</div>
<div class="control">
@ -68,13 +66,13 @@
<script setup lang="ts">
import BaseButton from '@/components/base/BaseButton.vue'
import CustomTransition from '@/components/misc/CustomTransition.vue'
import { closeWhenClickedOutside } from '@/helpers/closeWhenClickedOutside'
import { periodToSeconds, secondsToPeriod } from '@/helpers/time/period'
import {closeWhenClickedOutside} from '@/helpers/closeWhenClickedOutside'
import {periodToSeconds, secondsToPeriod} from '@/helpers/time/period'
import TaskReminderModel from '@/models/taskReminder'
import type { ITaskReminder } from '@/modelTypes/ITaskReminder'
import { REMINDER_PERIOD_RELATIVE_TO_TYPES, type IReminderPeriodRelativeTo } from '@/types/IReminderPeriodRelativeTo'
import { onMounted, onBeforeUnmount, reactive, ref, watch, type PropType } from 'vue'
import { useI18n } from 'vue-i18n'
import type {ITaskReminder} from '@/modelTypes/ITaskReminder'
import {REMINDER_PERIOD_RELATIVE_TO_TYPES, type IReminderPeriodRelativeTo} from '@/types/IReminderPeriodRelativeTo'
import {onMounted, onBeforeUnmount, reactive, ref, watch, type PropType} from 'vue'
import {useI18n} from 'vue-i18n'
const {t} = useI18n({useScope: 'global'})
@ -101,7 +99,7 @@ const periodInput = reactive({
})
onMounted(() => document.addEventListener('click', hidePeriodPopup))
onBeforeUnmount(() =>document.removeEventListener('click', hidePeriodPopup))
onBeforeUnmount(() => document.removeEventListener('click', hidePeriodPopup))
watch(
() => props.modelValue,
@ -138,6 +136,7 @@ function togglePeriodPopup() {
}
const periodPopup = ref<HTMLElement | null>(null)
function hidePeriodPopup(e: MouseEvent) {
if (isShowForm.value) {
closeWhenClickedOutside(e, periodPopup.value, close)
@ -150,6 +149,7 @@ function submitForm() {
}
const changed = ref(false)
function close() {
setTimeout(() => {
isShowForm.value = false
@ -171,13 +171,6 @@ function formatDuration(reminderPeriod: number): string {
('' + duration.minutes).padStart(2, '0')
}
function formatBeforeAfter(reminderPeriod: number): string {
if (reminderPeriod <= 0) {
return '&le;'
}
return '&gt;'
}
function formatRelativeTo(relativeTo: IReminderPeriodRelativeTo | null): string | null {
switch (relativeTo) {
case REMINDER_PERIOD_RELATIVE_TO_TYPES.DUEDATE:
@ -195,8 +188,8 @@ function formatRelativeTo(relativeTo: IReminderPeriodRelativeTo | null): string
<style lang="scss" scoped>
.input {
max-width: 70px;
width: 70px;
max-width: 5rem;
width: 4rem;
}
.close-button {

4
src/helpers/time/period.ts
View File

@ -3,7 +3,7 @@ import {SECONDS_A_DAY, SECONDS_A_HOUR, SECONDS_A_MINUTE} from '@/constants/date'
/**
* Convert time period given as seconds to days, hour, minutes, seconds
*/
export function secondsToPeriod(seconds: number) {
export function secondsToPeriod(seconds: number): {days: number, hours: number, minutes: number, seconds: number} {
dpschen marked this conversation as resolved Outdated
konrad commented 2023-03-16 19:16:50 +00:00
Outdated
Review
Copy Link

Can you add a type hint for the return value?

Can you add a type hint for the return value?
ce72 commented 2023-03-17 09:39:43 +00:00
Outdated
Review
Copy Link

ok

ok
dpschen commented 2023-03-20 16:48:28 +00:00
Outdated
Review
Copy Link

Couldn't we instead define the return via as const?
Also we don' really need the declaration of the variables but could inline those definitions in the returned object directly

Couldn't we instead define the return via `as const`? Also we don' really need the declaration of the variables but could inline those definitions in the returned object directly
ce72 commented 2023-03-20 20:33:22 +00:00
Outdated
Review
Copy Link

OK concerning the inlining. Can you please make a suggestion on how to replace the function using as const? My typescript might be not sufficient here.

OK concerning the inlining. Can you please make a suggestion on how to replace the function using `as const`? My typescript might be not sufficient here.
👍 1
const d = Math.floor(seconds / SECONDS_A_DAY)
const h = Math.floor(seconds % SECONDS_A_DAY / 3600)
const m = Math.floor(seconds % SECONDS_A_HOUR / 60)
@ -14,6 +14,6 @@ export function secondsToPeriod(seconds: number) {
/**
* Convert time period of days, hour, minutes, seconds to duration in seconds
*/
export function periodToSeconds(days: number, hours: number, minutes: number, seconds: number) : number{
export function periodToSeconds(days: number, hours: number, minutes: number, seconds: number): number {
return days * SECONDS_A_DAY + hours * SECONDS_A_HOUR + minutes * SECONDS_A_MINUTE + seconds
}

5
src/i18n/lang/en.json
View File

@ -720,6 +720,11 @@
"copiedto": "Copied To | Copied To"
}
},
"reminder": {
"hoursShort": "HH",
"minutesShort": "MM",
"daysShort": "d"
},
"repeat": {
"everyDay": "Every Day",
"everyWeek": "Every Week",