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

feat: hide quick add magic help behind a tooltip #3353

Merged
konrad merged 13 commits from feature/hide-quick-add-magic-help into main 2023-06-05 15:02:43 +00:00
Conversation 1 Commits 13 Files Changed 6 +51 -22
konrad commented 2023-04-05 07:13:24 +00:00
Owner
Copy Link

This PR changes the behaviour of the quick add magic tooltip below the task input field. The tooltip is now shown all the time until hidden. When it was hidden, that status is saved in local storage so that it persists across reloads.

I'm not 100% sure about this, because once hidden there's no way to open the help text modal (at all) because the message will not be shown again until logging out and in again. We could either add this as another setting in the user settings to allow users to change this afterwards, or add a way to open the modal with the text somewhere else.

Resolves #3253

This PR changes the behaviour of the quick add magic tooltip below the task input field. The tooltip is now shown all the time until hidden. When it was hidden, that status is saved in local storage so that it persists across reloads. I'm not 100% sure about this, because once hidden there's no way to open the help text modal (at all) because the message will not be shown again until logging out and in again. We could either add this as another setting in the user settings to allow users to change this afterwards, or add a way to open the modal with the text somewhere else. Resolves #3253
konrad added 1 commit 2023-04-05 07:13:26 +00:00
konrad requested review from dpschen 2023-04-05 07:13:29 +00:00
frederick commented 2023-04-05 07:18:43 +00:00
Member
Copy Link

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3353-feature-hide-quick-add-magic-hel--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!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3353-feature-hide-quick-add-magic-hel--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! > Beep boop, I'm a bot.
dpschen requested changes 2023-04-11 12:09:40 +00:00
src/components/tasks/add-task.vue
@ -1,5 +1,5 @@
<template>
<div class="task-add" ref="taskAdd">
<div class="task-add">
<div class="add-task__field field is-grouped">
dpschen commented 2023-04-05 09:58:54 +00:00
Member

Add a square BaseButton with nice padding (full height of one text line) to the top right of the textarea wrapper div. It has an info "?" icon inside. It has a tooltip with the quick add magic hint from the expandable. The same message is also via aria-label on the BaseButton. On click it will open the explanation directly.

Reuse the hover logic to show and hide the info button.

If I'm not mistaken the quickaddmagic.vue will be obsolete.

Add a square BaseButton with nice padding (full height of one text line) to the top right of the textarea wrapper div. It has an info "?" icon inside. It has a tooltip with the quick add magic hint from the expandable. The same message is also via `aria-label` on the BaseButton. On click it will open the explanation directly. Reuse the hover logic to show and hide the info button. If I'm not mistaken the `quickaddmagic.vue` will be obsolete.
✅ 1
konrad commented 2023-04-11 16:41:49 +00:00
Author
Owner

That makes sense.

That makes sense.
konrad commented 2023-04-11 17:11:16 +00:00
Author
Owner

I've now changed it so that it does what you're describing, but kept it in the existing component. That way the modal (and all logic to show it) stays in there and does not pollute the add task component.

I've now changed it so that it does what you're describing, but kept it in the existing component. That way the modal (and all logic to show it) stays in there and does not pollute the add task component.
dpschen commented 2023-04-11 20:32:27 +00:00
Member

The new version doesn't hide the "?" icon when the add-task is not hovered or focused.

To simplify the logic the delayEnter option of useElementHover can be used (didn't exist before).

I guess this could still be abstracted via a slot.
This way we would have an info component that shows the '?' always when focused or hovered.

The new version doesn't hide the "?" icon when the add-task is not hovered or focused. To simplify the logic the `delayEnter` option of [useElementHover](https://vueuse.org/core/useElementHover/) can be used (didn't exist before). I guess this could still be abstracted via a slot. This way we would have an info component that shows the '?' always when focused or hovered.
dpschen commented 2023-04-11 20:45:08 +00:00
Member

Contrast of the icon is really bad

Contrast of the icon is really bad
✅ 1
konrad commented 2023-04-12 08:06:14 +00:00
Author
Owner

I've improved the contrast

I've improved the contrast
👍 1
konrad commented 2023-04-12 08:08:32 +00:00
Author
Owner

The new version doesn't hide the "?" icon when the add-task is not hovered or focused.

IMHO it's fine to leave it like that. It's not in the way and would avoid flickering of another element when focusing

> The new version doesn't hide the "?" icon when the add-task is not hovered or focused. IMHO it's fine to leave it like that. It's not in the way and would avoid flickering of another element when focusing
dpschen commented 2023-04-12 12:01:56 +00:00
Member

[…] would avoid flickering of another element when focusing.

Fine with me. Flickering would be 'subtle' though, because I imagined a transition.

Now I think that the default is too strongly visible. What I head in mind was an alignment with e.g. the hover for lists / projects in the SingleLineInList task.

The color also needs a defined transition. If we add that transition we should add it for the icon in the front as well.

> […] would avoid flickering of another element when focusing. Fine with me. Flickering would be 'subtle' though, because I imagined a transition. Now I think that the default is too strongly visible. What I head in mind was an alignment with e.g. the hover for lists / projects in the SingleLineInList task. The color also needs a defined transition. If we add that transition we should add it for the icon in the front as well.
konrad commented 2023-04-12 16:31:49 +00:00
Author
Owner

Added a transition, seems to reduce the flickering by a lot. I also added a hover effect to highlight the icon when the input field is hovered.

Added a transition, seems to reduce the flickering by a lot. I also added a hover effect to highlight the icon when the input field is hovered.
dpschen commented 2023-04-12 18:06:40 +00:00
Member

added a hover effect to highlight the icon when the input field is hovered.

Only seems to work if the field is focused

> added a hover effect to highlight the icon when the input field is hovered. Only seems to work if the field is focused
konrad commented 2023-04-14 13:55:11 +00:00
Author
Owner

It works for me when the input is not focused. Which browser and version are you testing in?

It works for me when the input is not focused. Which browser and version are you testing in?
dpschen commented 2023-04-14 17:04:50 +00:00
Member

Might be that I didn't test the latest version. Works now

Might be that I didn't test the latest version. Works now
👍 1
dpschen marked this conversation as resolved
src/components/tasks/partials/quick-add-magic.vue Outdated
@ -3,6 +3,7 @@
<p class="help has-text-grey">
{{ $t('task.quickAddMagic.hint') }}.
<ButtonLink @click="() => visible = true">{{ $t('task.quickAddMagic.what') }}</ButtonLink>
<ButtonLink @click="() => emit('hide')" class="ml-1">{{ $t('task.quickAddMagic.hide') }}</ButtonLink>
dpschen commented 2023-04-05 09:45:24 +00:00
Member

Remove extra () =>

Remove extra `() =>`
✅ 1
dpschen marked this conversation as resolved
konrad added 1 commit 2023-04-11 17:09:36 +00:00
continuous-integration/drone/pr Build is passing Details
5e3b6c2c41
feat: move quick add magic to a popup behind an icon
konrad requested review from dpschen 2023-04-11 17:11:23 +00:00
dpschen reviewed 2023-04-11 20:27:48 +00:00
src/components/tasks/partials/quick-add-magic.vue Outdated
@ -1,9 +1,13 @@
<template>
<div v-if="mode !== 'disabled' && prefixes !== undefined">
dpschen commented 2023-04-11 20:27:48 +00:00
Member

Remove wrapper div and use template instead

Remove wrapper div and use template instead
✅ 1
konrad commented 2023-04-12 07:50:46 +00:00
Author
Owner

Ah, I thought you couldn't use a template as root node. TIL.
I guess that changed in vue 3?

Ah, I thought you couldn't use a `template` as root node. TIL. I guess that changed in vue 3?
konrad commented 2023-04-12 07:50:53 +00:00
Author
Owner

Now changed

Now changed
dpschen commented 2023-04-12 11:50:59 +00:00
Member

Ah, I thought you couldn't use a template as root node. TIL.
I guess that changed in vue 3?

Unsure, but yes might be!

> Ah, I thought you couldn't use a `template` as root node. TIL. > I guess that changed in vue 3? Unsure, but yes might be!
dpschen marked this conversation as resolved
dpschen reviewed 2023-04-11 20:37:08 +00:00
src/i18n/lang/en.json Outdated
@ -805,3 +805,2 @@
"quickAddMagic": {
"hint": "You can use Quick Add Magic",
"what": "What?",
"hint": "You can use Quick Add Magic. Click to learn more.",
dpschen commented 2023-04-11 20:37:08 +00:00
Member

Not happy with this last part of the text.
Don't like my current alternative:

Find out, how to use 'Quick Add Magic' here
Not happy with this last part of the text. Don't like my current alternative: ``` Find out, how to use 'Quick Add Magic' here
✅ 1
konrad commented 2023-04-12 08:05:55 +00:00
Author
Owner

I've changed it now.

I've changed it now.
dpschen marked this conversation as resolved
konrad added 1 commit 2023-04-12 07:50:04 +00:00
continuous-integration/drone/pr Build is passing Details
8dfc0ed55d
chore: remove wrapper div
konrad added 2 commits 2023-04-12 08:05:36 +00:00
continuous-integration/drone/pr Build is passing Details
23e5b9a103
fix: improve tooltip text
konrad added 1 commit 2023-04-12 16:25:11 +00:00
continuous-integration/drone/pr Build is passing Details
150f85ed9a
feat: add transition to input icons
konrad added 1 commit 2023-04-12 16:31:04 +00:00
continuous-integration/drone/pr Build is failing Details
b91671b6a5
feat: highlight hint icon when hovering the input
dpschen reviewed 2023-04-12 18:09:43 +00:00
src/components/tasks/partials/quick-add-magic.vue Outdated
@ -105,0 +117,4 @@
right: 0;
pointer-events: auto !important;
&.is-highlighted {
dpschen commented 2023-04-12 18:09:43 +00:00
Member

Is nesting even necessary if we already need to use !important for inheriting the color?

Is nesting even necessary if we already need to use `!important` for inheriting the color?
✅ 1
konrad commented 2023-04-14 13:56:01 +00:00
Author
Owner

It isn't, I added the nesting out of habit. Removed it now.

It isn't, I added the nesting out of habit. Removed it now.
dpschen marked this conversation as resolved
dpschen reviewed 2023-04-12 18:11:12 +00:00
src/i18n/lang/en.json Outdated
@ -805,3 +805,2 @@
"quickAddMagic": {
"hint": "You can use Quick Add Magic",
"what": "What?",
"hint": "Use magic prefixes to define due dates, assignees and other task properties.",
dpschen commented 2023-04-12 18:11:12 +00:00
Member

That's very good!

That's very good!
🎉 1
dpschen marked this conversation as resolved
dpschen reviewed 2023-04-12 18:13:09 +00:00
src/styles/theme/form.scss Outdated
@ -104,2 +104,4 @@
}
}
// Global style, because the rest of this styling is imported from bulma
dpschen commented 2023-04-12 18:13:09 +00:00
Member

Define this in the component we actually need it. This way we don't have to move this style when we remove bulma.
Also: do we overwrite some styles here?

If not: couldn't we add a new class for these icons instead, where used?

Define this in the component we actually need it. This way we don't have to move this style when we remove bulma. Also: do we overwrite some styles here? If not: couldn't we add a new class for these icons instead, where used?
konrad commented 2023-04-14 13:58:16 +00:00
Author
Owner

AFAIK we don't override these styles. We do use these classes in two places however, so that would mean we have to duplicate it if I put it where it's used?

AFAIK we don't override these styles. We do use these classes in two places however, so that would mean we have to duplicate it if I put it where it's used?
dpschen commented 2023-04-14 17:26:39 +00:00
Member

I think that would be better. We do import $transition globally, so there is already a common link.

Another way could be to define the transition via a CSS-property, e.g. --form-icon-transition (and set the value via the $transition), so the dependancy is solved via DOM hierarchy. Since CSS-properties are scoped by their nature it's also not polluting some global namespace.

While having an animation on icons makes sense I would no like to increase of use of classes created by bulma. So if we want to change something I think it would be better to create new classes for these elements – even if they already have classes.

Maybe a way of thinking of it is similar to js-hook classes. Sometimes one uses classes like js_is-open where that prefix makes clear that the class was set during runtime (I saw that pattern mostly in context of vanilla dom libs in combo with SSR). So just imagine that all bulma classes have bulma- prefixed and these are all of bulmas namespace. In order to decrease / minimise our dependency on bulma, which is generally a good thing todo we should not use those classes to add our styles, but instead add new ones.

I think that would be better. We do import `$transition` globally, so there is already a common link. Another way could be to define the transition via a CSS-property, e.g. `--form-icon-transition` (and set the value via the `$transition`), so the dependancy is solved via DOM hierarchy. Since CSS-properties are scoped by their nature it's also not polluting some global namespace. While having an animation on icons makes sense I would no like to increase of use of classes created by bulma. So if we want to change something I think it would be better to create new classes for these elements – even if they already have classes. Maybe a way of thinking of it is similar to `js`-hook classes. Sometimes one uses classes like `js_is-open` where that prefix makes clear that the class was set during runtime (I saw that pattern mostly in context of vanilla dom libs in combo with SSR). So just imagine that all bulma classes have `bulma-` prefixed and these are all of bulmas namespace. In order to decrease / minimise our dependency on bulma, which is generally a good thing todo we should not use those classes to add our styles, but instead add new ones.
konrad commented 2023-04-14 19:26:02 +00:00
Author
Owner

I've moved it to the components (duplicating it).

I would no like to increase of use of classes created by bulma.

That makes perfect sense. Ripping bulma out and replacing it with something else entirely is a whole separate project though, but it's a good idea to decrease our dependency on it, not increase it.

I've moved it to the components (duplicating it). > I would no like to increase of use of classes created by bulma. That makes perfect sense. Ripping bulma out and replacing it with something else entirely is a whole separate project though, but it's a good idea to decrease our dependency on it, not increase it.
dpschen reviewed 2023-04-12 18:14:13 +00:00
src/components/tasks/partials/quick-add-magic.vue Outdated
@ -105,0 +115,4 @@
<style lang="scss" scoped>
.show-helper-text {
right: 0;
pointer-events: auto !important;
dpschen commented 2023-04-12 18:14:13 +00:00
Member

Why pointer-events: auto?

Why `pointer-events: auto`?
konrad commented 2023-04-14 13:58:36 +00:00
Author
Owner

Because in BaseButton we set that to none which makes the button unclickable (and unhoverable).

Because in `BaseButton` we set that to `none` which makes the button unclickable (and unhoverable).
dpschen commented 2023-04-14 17:07:58 +00:00
Member

That can't be it, because there it's set to auto as well: https://kolaente.dev/vikunja/frontend/src/branch/main/src/components/base/BaseButton.vue#L115

That can't be it, because there it's set to `auto` as well: https://kolaente.dev/vikunja/frontend/src/branch/main/src/components/base/BaseButton.vue#L115
👍 1
dpschen commented 2023-04-14 17:12:10 +00:00
Member

Seems to be the .control.has-icons-left .icon class of bulma. Can you add a comment?

It also seems that the textarea could profit from has-icon-right so that there is enough padding for the button.

Seems to be the `.control.has-icons-left .icon` class of bulma. Can you add a comment? It also seems that the textarea could profit from `has-icon-right` so that there is enough padding for the button.
konrad commented 2023-04-14 19:20:16 +00:00
Author
Owner

Added a comment and more padding.

Added a comment and more padding.
konrad marked this conversation as resolved
dpschen reviewed 2023-04-12 18:15:04 +00:00
src/components/tasks/partials/quick-add-magic.vue Outdated
@ -105,0 +114,4 @@
<style lang="scss" scoped>
.show-helper-text {
right: 0;
dpschen commented 2023-04-12 18:15:04 +00:00
Member

Positioning should happen from outside. Remove right: 0 from here

Positioning should happen from outside. Remove `right: 0` from here
konrad commented 2023-04-14 14:03:20 +00:00
Author
Owner

Done

Done
konrad commented 2023-04-14 14:04:06 +00:00
Author
Owner

I had to use an unscoped style for this because I could not add a new class to the quick-add-magic component due to it being multi-node. Not sure if that's the best solution.

I had to use an unscoped style for this because I could not add a new class to the `quick-add-magic` component due to it being multi-node. Not sure if that's the best solution.
konrad marked this conversation as resolved
dpschen added 1 commit 2023-04-14 12:16:01 +00:00
continuous-integration/drone/pr Build is passing Details
df7e19bb73
fix: lint
konrad added 1 commit 2023-04-14 13:56:13 +00:00
continuous-integration/drone/pr Build is passing Details
fecc72fdcb
chore: remove nesting
konrad added 1 commit 2023-04-14 14:03:17 +00:00
continuous-integration/drone/pr Build is passing Details
0b1d33275a
chore: move positioning css
konrad added 1 commit 2023-04-14 19:19:21 +00:00
continuous-integration/drone/pr Build is passing Details
17233a27ad
chore: add comment on overriding
konrad added 1 commit 2023-04-14 19:20:11 +00:00
continuous-integration/drone/pr Build is passing Details
b2dbbea7f0
fix: add more padding to the textarea
konrad added 1 commit 2023-04-14 19:21:53 +00:00
continuous-integration/drone/pr Build is passing Details
cdc6886b17
chore: move styles to components
konrad force-pushed feature/hide-quick-add-magic-help from cdc6886b17 to 4b7a49a94a 2023-05-31 14:20:32 +00:00 Compare
konrad force-pushed feature/hide-quick-add-magic-help from 4b7a49a94a to 25c3b7bcbf 2023-06-05 14:20:45 +00:00 Compare
konrad changed title from feat: allow hiding the quick add magic help tooltip with a button to feat: hide quick add magic help behind a tooltip 2023-06-05 15:02:15 +00:00
konrad merged commit a988565227 into main 2023-06-05 15:02:43 +00:00
konrad deleted branch feature/hide-quick-add-magic-help 2023-06-05 15:02:44 +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
3 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#3353
No description provided.
Delete Branch "feature/hide-quick-add-magic-help"

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?