feat: add-task usability improvements #2767
Loading…
x
Reference in New Issue
Block a user
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!
a6a431a126
tod6741e601a
@ -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.
d6741e601a
toa782abbc57
@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.
a782abbc57
to41aba1554e
I added a collapsable now. It expands on focus somewhere in the modul or on hover with a slight delay.
41aba1554e
to7cdd6a3469
@ -0,0 +10,4 @@
@after-leave="afterLeave"
@leave-cancelled="leaveCancelled"
>
<div
The
initialHeight
might be currently broken. I still kept it in since I'm not sure how to remove that in a way that makes sense.7cdd6a3469
to63f1074681
@ -37,3 +35,1 @@
{{ errorMessage }}
</p>
<quick-add-magic v-else/>
<Expandable :open="errorMessage !== '' || taskAddFocused || taskAddHovered && debouncedTaskAddHovered">
By combining
taskAddHovered
withdebouncedTaskAddHovered
we 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
which
is deprecatedI think it resets the error message if any letter was pressed. (iirc the
change
event 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
useActiveElement
was always<body>
in betweenblur
andfocus
. 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-out
We use here easing gradients. Because it's visually the only thing that makes sense if you want to fade out content.
63f1074681
to40367468e8
@ -0,0 +65,4 @@
const { display } = el.style // save display property
el.style.display = 'block' // Make it visible
const height = `${el.scrollHeight}px` // Get its height
// el.style.display = ''; // Hide it again
I think this comment can be removed.
@ -0,0 +117,4 @@
removeHeight(el)
}
function leaveCancelled(el: HTMLElement) {
Can you group all functions which just call
removeHeight
like 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-0
utility 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 !== undefined
to theavailable
computed? Or better not?I saw the available as
isNotDisabled
which 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.
40367468e8
to3927965e26
@konrad can you check again?
feat: add-task usability improvementsto WIP: feat: add-task usability improvements3927965e26
to634bdc8f02
WIP: 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-duration
but with 150ms it felt to short.@ -73,1 +79,4 @@
const taskAdd = ref<HTMLTextAreaElement | null>(null)
// enable only if we don't have a modal
What 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.
634bdc8f02
toa0797570ed