feat: add-task usability improvements #2767
No reviewers
konrad
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
3 Participants
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#2767
Loading…
Reference in New Issue
No description provided.
Delete Branch "dpschen/frontend:feature/improve-add-task"
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?
Hi dpschen!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://2767-feature-improve-add-task--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!
a6a431a126tod6741e601a@ -3,7 +3,6 @@<div class="field is-grouped"><p class="control has-icons-left is-expanded"><textarea:disabled="loading || undefined"You can always write in this field. But you may not be able so submit.
This makes the initial loading of the home page calmer.
@ -208,0 +209,4 @@opacity: 0;transition: opacity $transition;}.task-add:has(.add-task-textarea:focus) .quick-add-magic {By using
:has(.add-task-textarea:focus)we check if the input has focus.I really like that idea, but it looks like it does not work:
Image doesn't load.
Which browser?
If it's not transpiled then we still have an issue with postcss-preset-env:
https://github.com/csstools/postcss-plugins/tree/main/plugins/css-has-pseudo
Firefox
Turned out that using focus-within was the smartest solution.
Because the user should be able to tab to the "What" button.
With the
:has(.add-task:focus)solution the element would be hidden when the textarea loses focus.Seems to work now!
@ -1,5 +1,5 @@<template><div v-if="available"><div v-if="available && prefixes !== undefined">This is mostly for typescript, because it didn't get that prefixes won't be used if available is false.
d6741e601atoa782abbc57@konrad please recheck
I'm wondering if we could improve the empty space here when the help text ist not shown?
I kept that for now by intend. Reason being that I didn't want the content to jump too much when the field gets focus.
I do still have a component that animates height smoothly somewhere. We could add that at a later stage.
That makes sense. Still I think we should do something about the space, it just does not look great right now
I thought about this again I'm not sure I would like the animation tbh. Reason beeing that the whole list would move up and down. Right now we show and hide this on hover. Maybe that was too much, but if we move the list up an down only when we hover it seems a bit much.
I agree about the animation. Not sure what we could do about this instead? Maybe hide the notice completely after showing it a few times?
A user might want to look something up at a later time. Where would you find it adter it has been hidden?
Good point. We could just add it to the menu or help document, but I feel like there should be a better way to it.
a782abbc57to41aba1554eI added a collapsable now. It expands on focus somewhere in the modul or on hover with a slight delay.
41aba1554eto7cdd6a3469@ -0,0 +10,4 @@@after-leave="afterLeave"@leave-cancelled="leaveCancelled"><divThe
initialHeightmight be currently broken. I still kept it in since I'm not sure how to remove that in a way that makes sense.7cdd6a3469to63f1074681@ -37,3 +35,1 @@{{ errorMessage }}</p><quick-add-magic v-else/><Expandable :open="errorMessage !== '' || taskAddFocused || taskAddHovered && debouncedTaskAddHovered">By combining
taskAddHoveredwithdebouncedTaskAddHoveredwe make sure that the content is shown on hover, but only if it's debounced. We make also sure that if there is no focus or error and the mouse leaves the area the content will immediately be hidden.@ -74,0 +78,4 @@const taskAdd = ref<HTMLTextAreaElement | null>(null)// enable only if we don't have a modal// onStartTyping(() => {This is really cool. But I can only enable this in the moment that we have a reliable way to detect if there is a modal open. E.g. This fails currently for the quick-actions.
@ -76,3 +95,3 @@function resetEmptyTitleError(e) {function resetEmptyTitleError(e: KeyboardEvent) {if ((e.which <= 90 && e.which >= 48 || e.which >= 96 && e.which <= 105)I don't get what this does. Also
whichis deprecatedI think it resets the error message if any letter was pressed. (iirc the
changeevent on the checkbox triggers even if you press something like esc or shift).I still don't get this. This is called from a
<textarea>not from a checkbox.Also I understand where the specific values from the range come from / which keys they stand for. The range includes many more then just two codes (as implied by your "esc or shift" comment).
Maybe we can actually remove this? (in a follow-up PR)
@ -0,0 +15,4 @@() => window?.document.activeElement as T | null | undefined,)function waitForRelatedTarget(event: FocusEvent) {I had to modify this since
useActiveElementwas always<body>in betweenblurandfocus. That brokeuseFocusWithin. Made a pull request https://github.com/vueuse/vueuse/pull/2540Since this is now merged and released in vueuse, maybe we can get rid of the code in here?
I think so, yes! Will check if it works :)
Oooops. Seems like I actually broke it instead :D
https://github.com/vueuse/vueuse/pull/2581
Tried to fix it again with https://github.com/vueuse/vueuse/pull/2600.
But I guess it will take a bit until it's merged.
Let's not make this pull request dependant on the merge.
Since it's merged now I'm adding the update.
@ -0,0 +132,4 @@.expandable-slide-enter-active,.expandable-slide-leave-active {transition:opacity $transition-time ease-in-quint,These functions are possible with the postcss-easings plugin. There is somewhere a working draft for this (couldn't find, but I know it exists).
@ -0,0 +154,4 @@background-image: linear-gradient(to bottom,transparent,ease-in-outWe use here easing gradients. Because it's visually the only thing that makes sense if you want to fade out content.
63f1074681to40367468e8@ -0,0 +65,4 @@const { display } = el.style // save display propertyel.style.display = 'block' // Make it visibleconst height = `${el.scrollHeight}px` // Get its height// el.style.display = ''; // Hide it againI think this comment can be removed.
@ -0,0 +117,4 @@removeHeight(el)}function leaveCancelled(el: HTMLElement) {Can you group all functions which just call
removeHeightlike this one? There's probably a good reason to define them with different names even though they all do the same, but it would be nice if they were grouped together so that it's all obvious in one place.I kept the orignal hook order of the vue transition component. Is it fine if I add some comments?
Yss, should be fine then.
Todo: add comments
Done.
@ -195,1 +214,3 @@.task-add {.task-add,// overwrite bulma styles.task-add .add-task__field {If that's the whole purpose of the selector we might as well just us an
mb-0utility on the div instead.I would agree if we would add space here and not reset bulma styles.
But since we reset something that bulma did, where I'm not sure if that makes so much sence in the first place I would keep it here, so that if we remove this part of bulma we can easily remove the rules.
@ -1,5 +1,5 @@<template><div v-if="available"><div v-if="available && prefixes !== undefined">Can't we move
prefixes !== undefinedto theavailablecomputed? Or better not?I saw the available as
isNotDisabledwhich is what is value is.Will rename a bit and maybe add a third computed to make it clearer.
I removed the computed and put the calculation inline instead.
40367468e8to3927965e26@konrad can you check again?
feat: add-task usability improvementsto WIP: feat: add-task usability improvements3927965e26to634bdc8f02WIP: feat: add-task usability improvementsto feat: add-task usability improvements@ -0,0 +132,4 @@</script><style lang="scss" scoped>$transition-time: 300ms;Don't we already have a variable for this?
There is the global
$transition-durationbut with 150ms it felt to short.@ -73,1 +79,4 @@const taskAdd = ref<HTMLTextAreaElement | null>(null)// enable only if we don't have a modalWhat about this?
See: #2767 (comment)
I think the current animation looks great and is a good middleground. There might be someone complaining afterwards but I think we'll just fix it in that case once it's merged and released.
Should be fine to merge once the CI passes.
Sounds good! Was indeed more complex than anticipated.
Can you rebase again in main? Then the tests should pass.
634bdc8f02toa0797570ed