feat: task checklist improvements #797

Merged
konrad merged 5 commits from feature/checklist-improvements into main 2021-09-29 18:31:15 +00:00
9 changed files with 232 additions and 37 deletions

View File

@ -53,6 +53,7 @@ import hljs from 'highlight.js/lib/common'
import AttachmentModel from '../../models/attachment'
import AttachmentService from '../../services/attachment'
import {findCheckboxesInText} from '../../helpers/checklistFromText'
export default {
name: 'editor',
@ -303,36 +304,7 @@ export default {
return str.substr(0, index) + replacement + str.substr(index + replacement.length)
},
findNthIndex(str, n) {
const searchLength = 6
const listPrefixes = ['*', '-']
let inChecked, inUnchecked, startIndex = 0
// We're building an array with all checkboxes, checked or unchecked.
// I've found this to be the best way to always get the results I need.
// The difficulty without an index is that we need to get all checkboxes, checked and unchecked
// and calculate our index based off that to compare it and find the checkbox we need.
let checkboxes = []
// Searching in two different loops for each search term since that is way easier and more predicatble
// More "intelligent" solutions sometimes don't have all values or duplicates.
// Because we're sorting and removing duplicates of them, we can safely put everything in one giant array.
listPrefixes.forEach(pref => {
while ((inChecked = str.indexOf(`${pref} [x]`, startIndex)) > -1) {
checkboxes.push(inChecked)
startIndex = startIndex + searchLength
}
startIndex = 0
while ((inUnchecked = str.indexOf(`${pref} [ ]`, startIndex)) > -1) {
checkboxes.push(inUnchecked)
startIndex = startIndex + searchLength
}
})
checkboxes.sort((a, b) => a - b)
checkboxes = checkboxes.filter((v, i, s) => s.indexOf(v) === i && v > -1)
const checkboxes = findCheckboxesInText(str)
return checkboxes[n]
},
renderPreview() {
@ -435,7 +407,7 @@ export default {
console.debug(index, this.text.substr(index, 9))
const listPrefix = this.text.substr(index, 1)
if (checked) {
this.text = this.replaceAt(this.text, index, `${listPrefix} [x] `)
} else {
@ -475,7 +447,7 @@ export default {
input[type="checkbox"] {
margin-right: .5rem;
}
&.has-checkbox {
margin-left: -2em;
list-style: none;

View File

@ -0,0 +1,60 @@
<template>
<span v-if="checklist.total > 0" class="checklist-summary">
<svg width="12" height="12">
<circle stroke-width="2" fill="transparent" cx="50%" cy="50%" r="5"></circle>
<circle stroke-width="2" stroke-dasharray="31" :stroke-dashoffset="checklistCircleDone"
stroke-linecap="round" fill="transparent" cx="50%" cy="50%" r="5"></circle>
</svg>
<span>
{{ $t(checklist.total === checklist.checked ? 'task.checklistAllDone' : 'task.checklistTotal', checklist) }}
</span>
</span>
</template>
<script>
import {getChecklistStatistics} from '../../../helpers/checklistFromText'
export default {
name: 'checklist-summary',
props: {
task: {
required: true,
},
},
computed: {
checklist() {
return getChecklistStatistics(this.task.description)
},
checklistCircleDone() {
const r = 5
const c = Math.PI * (r * 2)
const progress = this.checklist.checked / this.checklist.total * 100
return ((100 - progress) / 100) * c
},
},
}
</script>
<style scoped lang="scss">
.checklist-summary {
color: $grey-500;
display: inline-flex;
align-items: center;
svg {
konrad marked this conversation as resolved Outdated

No need to nest svg and circle in scoped styles.

No need to nest `svg` and `circle` in scoped styles.

Fixed.

Fixed.
transform: rotate(-90deg);
transition: stroke-dashoffset 0.35s;
margin-right: .25rem;
circle {
stroke: $grey-400;
&:last-child {
stroke: $primary;
}
}
}
}
</style>

View File

@ -59,6 +59,7 @@
<icon icon="align-left"/>
</span>
</span>
<checklist-summary :task="task"/>
</router-link>
<progress
class="progress is-small"
@ -94,6 +95,7 @@ import Fancycheckbox from '../../input/fancycheckbox'
import DeferTask from './defer-task'
import {closeWhenClickedOutside} from '@/helpers/closeWhenClickedOutside'
import {playPop} from '@/helpers/playPop'
import ChecklistSummary from './checklist-summary'
export default {
name: 'singleTaskInList',
@ -105,6 +107,7 @@ export default {
}
},
components: {
ChecklistSummary,
DeferTask,
Fancycheckbox,
User,
@ -172,10 +175,10 @@ export default {
this.task = t
this.$emit('task-updated', t)
this.$message.success({
message: this.task.done ?
this.$t('task.doneSuccess') :
this.$t('task.undoneSuccess'),
}, [{
message: this.task.done ?
this.$t('task.doneSuccess') :
this.$t('task.undoneSuccess'),
}, [{
title: 'Undo',
callback: () => {
this.task.done = !this.task.done

View File

@ -0,0 +1,91 @@
import {findCheckboxesInText, getChecklistStatistics} from './checklistFromText'
describe('Find checklists in text', () => {
it('should find no checkbox', () => {
const text: string = 'Lorem Ipsum'
const checkboxes = findCheckboxesInText(text)
expect(checkboxes).toHaveLength(0)
})
it('should find multiple checkboxes', () => {
const text: string = `* [ ] Lorem Ipsum
* [ ] Dolor sit amet
Here's some text in between
* [x] Dolor sit amet
- [ ] Dolor sit amet`
const checkboxes = findCheckboxesInText(text)
expect(checkboxes).toHaveLength(4)
expect(checkboxes[0]).toBe(0)
expect(checkboxes[1]).toBe(18)
expect(checkboxes[2]).toBe(69)
})
it('should find one checkbox with *', () => {
const text: string = '* [ ] Lorem Ipsum'
const checkboxes = findCheckboxesInText(text)
expect(checkboxes).toHaveLength(1)
expect(checkboxes[0]).toBe(0)
})
it('should find one checkbox with -', () => {
const text: string = '- [ ] Lorem Ipsum'
const checkboxes = findCheckboxesInText(text)
expect(checkboxes).toHaveLength(1)
expect(checkboxes[0]).toBe(0)
})
it('should find one checked checkbox with *', () => {
const text: string = '* [x] Lorem Ipsum'
const checkboxes = findCheckboxesInText(text)
expect(checkboxes).toHaveLength(1)
expect(checkboxes[0]).toBe(0)
})
it('should find one checked checkbox with -', () => {
const text: string = '- [x] Lorem Ipsum'
const checkboxes = findCheckboxesInText(text)
expect(checkboxes).toHaveLength(1)
expect(checkboxes[0]).toBe(0)
})
})
describe('Get Checklist Statistics in a Text', () => {
it('should find no checkbox', () => {
const text: string = 'Lorem Ipsum'
const stats = getChecklistStatistics(text)
expect(stats.total).toBe(0)
})
it('should find one checkbox', () => {
const text: string = '* [ ] Lorem Ipsum'
const stats = getChecklistStatistics(text)
expect(stats.total).toBe(1)
expect(stats.checked).toBe(0)
})
it('should find one checked checkbox', () => {
const text: string = '* [x] Lorem Ipsum'
const stats = getChecklistStatistics(text)
expect(stats.total).toBe(1)
expect(stats.checked).toBe(1)
})
it('should find multiple mixed and matched', () => {
const text: string = `* [ ] Lorem Ipsum
* [ ] Dolor sit amet
* [x] Dolor sit amet
- [x] Dolor sit amet
Here's some text in between
* [x] Dolor sit amet
- [ ] Dolor sit amet`
const stats = getChecklistStatistics(text)
expect(stats.total).toBe(6)
expect(stats.checked).toBe(3)
})
})

View File

@ -0,0 +1,53 @@
const checked = '[x]'
konrad marked this conversation as resolved
Review

searchLength, listPrefixes and unchecked are not used anymore. rest of this file looks good :) cool!

`searchLength`, `listPrefixes` and `unchecked` are not used anymore. rest of this file looks good :) cool!
Review

Good catch :) I've removed them.

Good catch :) I've removed them.
interface CheckboxStatistics {
total: number
checked: number
}
interface MatchedCheckboxes {
checked: number[]
unchecked: number[]
}
const getCheckboxesInText = (text: string): MatchedCheckboxes => {
const regex = /[*-] \[[ x]]/g
let match
const checkboxes: MatchedCheckboxes = {
checked: [],
unchecked: [],
}
while ((match = regex.exec(text)) !== null) {
if (match[0].endsWith(checked)) {
konrad marked this conversation as resolved Outdated

I might be wrong because of poor regex logic knowledge, maybe it's possible to use a simple one here:
https://regex101.com/r/WuD5uS/1

In combination with https://stackoverflow.com/a/2295681/15522256 a single loop + analysing the found match would be enough.


That said: Ignore if comment is annoying:
I'm forcefully trying to find a good usecase for regex, learn something new and am aware that the current solution is more than sufficient

I might be wrong because of poor regex logic knowledge, maybe it's possible to use a simple one here: https://regex101.com/r/WuD5uS/1 In combination with https://stackoverflow.com/a/2295681/15522256 a single loop + analysing the found match would be enough. --- That said: Ignore if comment is annoying: I'm forcefully trying to find a good usecase for regex, learn something new and am aware that the current solution is more than sufficient

I've modified it now to use a regex. The original implementation from the editor component was from a time where I didn't really knew regex, I think using one here makes it a lot simpler and easier to read.

I've modified it now to use a regex. The original implementation from the editor component was from a time where I didn't really knew regex, I think using one here makes it a lot simpler and easier to read.
checkboxes.checked.push(match.index)
} else {
checkboxes.unchecked.push(match.index)
}
}
return checkboxes
}
/**
* Returns the indices where checkboxes start and end in the given text.
*
* @param text
*/
export const findCheckboxesInText = (text: string): number[] => {
const checkboxes = getCheckboxesInText(text)
return [
...checkboxes.checked,
...checkboxes.unchecked,
].sort()
}
export const getChecklistStatistics = (text: string): CheckboxStatistics => {
konrad marked this conversation as resolved Outdated

Independant of the regex: while looping through the loop we already know if a checkbox is checked or not. Why not save that info and return it by default. If you don't want to return an object in the original function you could write a second one that wraps the loop and omits all other info than checkboxes.

Independant of the regex: while looping through the loop we already know if a checkbox is checked or not. Why not save that info and return it by default. If you don't want to return an object in the original function you could write a second one that wraps the loop and omits all other info than `checkboxes`.

I've modified that as well and moved the logic into another (unexported) function which returns the checked and unchecked independently from each other. I'm then using that function in the two other ones.

The original implementation in the editor component came out of a lot of trial and error and, because I didn't have any tests at that time either, was at the end a "do not touch, magic" kind of function. I don't even recall why I had two loops and then weren't even using the result. This new version is a lot cleaner.

I've modified that as well and moved the logic into another (unexported) function which returns the checked and unchecked independently from each other. I'm then using that function in the two other ones. The original implementation in the editor component came out of a lot of trial and error and, because I didn't have any tests at that time either, was at the end a "do not touch, magic" kind of function. I don't even recall why I had two loops and then weren't even using the result. This new version is a lot cleaner.

Looks good :)

Looks good :)
const checkboxes = getCheckboxesInText(text)
return {
total: checkboxes.checked.length + checkboxes.unchecked.length,
checked: checkboxes.checked.length,
}
}

View File

@ -493,6 +493,8 @@
"doneSuccess": "The task was successfully marked as done.",
"undoneSuccess": "The task was successfully un-marked as done.",
"openDetail": "Open task detail view",
"checklistTotal": "{checked} of {total} tasks",
"checklistAllDone": "{total} tasks",
"show": {
"titleCurrent": "Current Tasks",
"titleDates": "Tasks from {from} until {to}",

View File

@ -10,6 +10,7 @@
.subtitle {
color: $grey-500;
margin-bottom: 1rem;
a {
color: $grey-800;
@ -178,6 +179,10 @@
color: $grey-500;
text-align: right;
}
.checklist-summary {
margin-left: .25rem;
}
}
.link-share-container:not(.has-background) .task-view {

View File

@ -195,6 +195,11 @@
border-bottom-color: $grey-300;
}
}
.checklist-summary {
padding-left: .5rem;
font-size: .9rem;
}
.progress {
width: 50px;

View File

@ -8,9 +8,11 @@
{{ getListTitle(parent.list) }}
</router-link>
</h6>
<checklist-summary :task="task"/>
<!-- Content and buttons -->
<div class="columns">
<div class="columns mt-2">
<!-- Content -->
<div :class="{'is-two-thirds': canWrite}" class="column">
<div class="columns details">
@ -445,10 +447,12 @@ import TaskSubscription from '@/components/misc/subscription.vue'
import {CURRENT_LIST} from '@/store/mutation-types'
import {uploadFile} from '@/helpers/attachments'
import ChecklistSummary from '../../components/tasks/partials/checklist-summary'
export default {
name: 'TaskDetailView',
components: {
ChecklistSummary,
TaskSubscription,
Datepicker,
ColorPicker,