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

feat(natural language): make natural language prefixes configurable #795

Merged
konrad merged 10 commits from feature/natural-language-config into main 2021-09-29 18:30:56 +00:00
Conversation 2 Commits 10 Files Changed 7 +192 -89
konrad commented 2021-09-26 14:48:36 +00:00
Owner
Copy Link

This PR adds a user setting to make the natural language prefixes configurable. It also adds a new mode "Vikunja" that should feel more intuitive and is different from the current mode which was basically a 1:1 copy of Todoist's prefixes. The new "Vikunja" mode is now the default.

Additionally, it adds a disable option to disable parsing completely.

This PR adds a user setting to make the natural language prefixes configurable. It also adds a new mode "Vikunja" that should feel more intuitive and is different from the current mode which was basically a 1:1 copy of Todoist's prefixes. The new "Vikunja" mode is now the default. Additionally, it adds a disable option to disable parsing completely.
konrad commented 2021-09-26 14:48:51 +00:00
Author
Owner
Copy Link

@dpschen Would you mind taking a look at this?

@dpschen Would you mind taking a look at this?
👍 1
dpschen commented 2021-09-26 14:50:51 +00:00
Member
Copy Link

Will do this evening :)

Will do this evening :)
👍 1
konrad force-pushed feature/natural-language-config from 4d09532fd7 to d30a1e4083 2021-09-26 16:24:05 +00:00 Compare
dpschen approved these changes 2021-09-26 22:24:24 +00:00
src/components/tasks/partials/quick-add-magic.vue Outdated
@ -76,3 +79,4 @@
data() {
return {
visible: false,
available: getQuickAddMagicMode() !== 'disabled',
dpschen commented 2021-09-26 22:20:15 +00:00
Member

mode should be read out in data once. So that it doesn't has to be repeated – saving one access to localStorage.

`mode` should be read out in data once. So that it doesn't has to be repeated – saving one access to localStorage.
konrad commented 2021-09-27 19:35:47 +00:00
Author
Owner

Done!

Done!
👍 1
konrad marked this conversation as resolved
src/components/tasks/partials/quick-add-magic.vue Outdated
@ -77,2 +80,4 @@
return {
visible: false,
available: getQuickAddMagicMode() !== 'disabled',
prefixes: {},
dpschen commented 2021-09-26 22:22:17 +00:00
Member

..Then prefixes could be a computed.

..Then prefixes could be a computed.
konrad commented 2021-09-27 19:35:55 +00:00
Author
Owner

Done!

Done!
konrad marked this conversation as resolved
src/helpers/quickAddMagicMode.ts Outdated
@ -0,0 +2,4 @@
const key = 'quickAddMagicMode'
export const setQuickAddMagicMode = (mode: string) => {
dpschen commented 2021-09-26 16:01:01 +00:00
Member

Since this basically already is an abstraction around localStorage, how about going the full way and create it already?

This way we can use this in the future everywhere where localStorage is used.

Since this basically already is an abstraction around localStorage, how about going the full way and create it already? This way we can use this in the future everywhere where localStorage is used.
konrad commented 2021-09-27 19:32:59 +00:00
Author
Owner

Yes, I'd like to do this but there's more things which need migration. An abstraction would ideally be used everywhere there currently is an access to localStorage and also handle things like cleanup on logout and such in one central place. That's on the radar but out of scope for this PR IMHO.

Yes, I'd like to do this but there's more things which need migration. An abstraction would ideally be used everywhere there currently is an access to localStorage and also handle things like cleanup on logout and such in one central place. That's on the radar but out of scope for this PR IMHO.
dpschen commented 2021-09-28 07:19:16 +00:00
Member

Makes sense 👍

Makes sense 👍
konrad marked this conversation as resolved
src/helpers/quickAddMagicMode.ts Outdated
@ -0,0 +7,4 @@
}
export const getQuickAddMagicMode = (): PrefixMode => {
return localStorage.getItem(key) ?? PrefixMode.Todoist
dpschen commented 2021-09-26 22:10:17 +00:00
Member

The default prefix should be defined together with the other prefixes.

The default prefix should be defined together with the other prefixes.
konrad commented 2021-09-27 19:58:05 +00:00
Author
Owner

Done, renamed the Vikunja one.

Done, renamed the `Vikunja` one.
👍 1
konrad marked this conversation as resolved
src/modules/parseTaskText.ts Outdated
@ -6,2 +4,2 @@
const PRIORITY_PREFIX: string = '!'
const ASSIGNEE_PREFIX: string = '+'
export const VIKUNJA_PREFIXES: Prefixes = {
label: '~',
dpschen commented 2021-09-26 22:16:32 +00:00
Member

Writing ~ needs two keys on a mac with german keyboard. How about *?
Or does the tilde have an origin that I'm not aware of?

Writing `~` needs two keys on a mac with german keyboard. How about `*`? Or does the tilde have an origin that I'm not aware of?
konrad commented 2021-09-27 19:42:59 +00:00
Author
Owner

The tilde doesn't have a special meaning. I think it makes sense have something easily reachable, changed to *.

The tilde doesn't have a special meaning. I think it makes sense have something easily reachable, changed to `*`.
👍 1
konrad marked this conversation as resolved
src/modules/parseTaskText.ts Outdated
@ -45,0 +67,4 @@
return result
}
let prefixes: Prefixes = TODOIST_PREFIXES
dpschen commented 2021-09-26 21:55:12 +00:00
Member

Maybe it's possible to define the prefixes as object so that they can be referenced similar to this:

PREFIXES[prefixId]

If prefixId === undefined it would be equal to disabled.

In case you know the one you want to reference you could still do that:

PREFIXES.DEFAULT // would be vikunja
PREFIXES.TODOIST
Maybe it's possible to define the prefixes as object so that they can be referenced similar to this: ```js PREFIXES[prefixId] ``` If `prefixId === undefined` it would be equal to disabled. In case you know the one you want to reference you could still do that: ```js PREFIXES.DEFAULT // would be vikunja PREFIXES.TODOIST ```
konrad commented 2021-09-27 19:57:48 +00:00
Author
Owner

Done that, but kept an explicit option for disabled so that we can return it in the getQuickAddMagicMode method.

Done that, but kept an explicit option for disabled so that we can return it in the `getQuickAddMagicMode` method.
konrad marked this conversation as resolved
src/views/user/Settings.vue Outdated
@ -80,0 +84,4 @@
</span>
<div class="select ml-2">
<select v-model="quickAddMagicMode">
<option value="disabled">{{ $t('user.settings.quickAddMagic.disabled') }}</option>
dpschen commented 2021-09-26 22:05:39 +00:00
Member

The list of possible options could be automatically created by the available prefixes.
That way the prefixes are just defined at one place.

<option v-for="set in prefixes" :key="set" :value="set">{{ $t(`user.settings.quickAddMagic.${set}`) }}</option>

Also: is disabled an option that should even be available?
Maybe it's fine already to chose between the different sets.

The list of possible options could be automatically created by the available prefixes. That way the prefixes are just defined at one place. ```vue <option v-for="set in prefixes" :key="set" :value="set">{{ $t(`user.settings.quickAddMagic.${set}`) }}</option> ``` Also: is disabled an option that should even be available? Maybe it's fine already to chose between the different sets.
konrad commented 2021-09-27 20:03:11 +00:00
Author
Owner

I think there should be an option to disable it. Ideally there would not be a need for it, but until the natural language parsing is perfect (it will never be), there should be an option to disable it in cases where it generates a lot of false positives 🙂

I think there should be an option to disable it. Ideally there would not be a need for it, but until the natural language parsing is perfect (it will never be), there should be an option to disable it in cases where it generates a lot of false positives 🙂
👍 1
dpschen commented 2021-09-28 07:32:16 +00:00
Member

Oh yes that makes sense! Didn't think about that.

Oh yes that makes sense! Didn't think about that.
konrad marked this conversation as resolved
konrad added 1 commit 2021-09-27 19:35:43 +00:00
konrad added 1 commit 2021-09-27 19:39:07 +00:00
konrad added 1 commit 2021-09-27 19:42:59 +00:00
konrad added 1 commit 2021-09-27 19:55:21 +00:00
konrad added 1 commit 2021-09-27 19:57:27 +00:00
konrad added 1 commit 2021-09-27 20:01:53 +00:00
konrad requested review from dpschen 2021-09-27 20:03:16 +00:00
dpschen reviewed 2021-09-28 07:30:36 +00:00
src/helpers/quickAddMagicMode.ts Outdated
@ -0,0 +9,4 @@
export const getQuickAddMagicMode = (): PrefixMode => {
const mode = localStorage.getItem(key)
switch (mode) {
dpschen commented 2021-09-28 07:30:36 +00:00
Member 
return PrefixMode[mode] || PrefixMode.Disabled

Would make it future proof by supporting more modes by default.

```js return PrefixMode[mode] || PrefixMode.Disabled ``` Would make it future proof by supporting more modes by default.
konrad commented 2021-09-29 18:17:26 +00:00
Author
Owner

That's a nice simplification. Added 👍

That's a nice simplification. Added 👍
konrad commented 2021-09-29 20:43:38 +00:00
Author
Owner

Actually, this always disabled quick add magic, no matter the value of mode - reverted this in 4a1b402e62.

Not sure why this was happening, I guess it had something to do with the way typescript enums work.

Actually, this always disabled quick add magic, no matter the value of `mode` - reverted this in https://kolaente.dev/vikunja/frontend/commit/4a1b402e62c962e55c7d96011620e86f1889106e. Not sure why this was happening, I guess it had something to do with the way typescript enums work.
dpschen commented 2021-09-30 08:54:44 +00:00
Member

Makes sense! It was pseudocode from memory. Should have been:

return PREFIXES[mode] || PREFIXES[PrefixMode.Disabled]
Makes sense! It was pseudocode from memory. Should have been: ```js return PREFIXES[mode] || PREFIXES[PrefixMode.Disabled] ```
konrad marked this conversation as resolved
konrad added 1 commit 2021-09-29 18:16:52 +00:00
continuous-integration/drone/pr Build is passing Details
4fabd288e8
chore: simplify getting the quick add magic mode
konrad merged commit d47b13647e into main 2021-09-29 18:30:56 +00:00
konrad deleted branch feature/natural-language-config 2021-09-29 18:30:56 +00:00
dpschen refused to review 2021-09-30 08:54:54 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
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
2 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#795
No description provided.
Delete Branch "feature/natural-language-config"

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?