feat: color the task color button when the task has a color set #2331

Merged
konrad merged 5 commits from feature/color-task-color-button into main 2022-09-15 12:46:13 +00:00
Owner

Partially releated to #811 (comment)

Partially releated to https://kolaente.dev/vikunja/frontend/issues/811#issuecomment-32592
konrad added 1 commit 2022-09-07 15:58:28 +00:00
konrad requested review from dpschen 2022-09-07 15:58:57 +00:00
dpschen was assigned by konrad 2022-09-07 15:59:03 +00:00
Member

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2331-feature-color-task-color-button--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://2331-feature-color-task-color-button--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.
Member

TBH I'm not a big fan of this change.
In my point of view the buttons in the right bar should have a similar appearance.
Also when you e.g. select red as color the button will get red. Same with green.
Red and green buttons already have a semantic (positive, confirm vs danger, delete, etc).

Our buttons doesn't have to do anything with this semantic.

Couldn't we instead use the bullet that we use everywhere else as color indicator?

TBH I'm not a big fan of this change. In my point of view the buttons in the right bar should have a similar appearance. Also when you e.g. select red as color the button will get red. Same with green. Red and green buttons already have a semantic (positive, confirm vs danger, delete, etc). Our buttons doesn't have to do anything with this semantic. Couldn't we instead use the bullet that we use everywhere else as color indicator?
Member

Regardless of my view of this implementation:

I found some issues when setting the color to blue when another color was set before: The color of the set color button doesn't keep in sync.

Blue seems to be the same as 'no color'. Could also be that this is unrealated to this pull request (didn't check the normal behavior here).

Regardless of my view of this implementation: I found some issues when setting the color to blue when another color was set before: The color of the set color button doesn't keep in sync. Blue seems to be the same as 'no color'. Could also be that this is unrealated to this pull request (didn't check the normal behavior here).
Author
Owner

I think coloring the dot is a good tradeoff.

I think coloring the dot is a good tradeoff.
konrad force-pushed feature/color-task-color-button from 60a79996f4 to 51c806c12b 2022-09-14 16:44:06 +00:00 Compare
konrad added 1 commit 2022-09-14 16:56:57 +00:00
Author
Owner

I ended up coloring the icon instead of adding a color dot to the button. I think that avoids the button having too many (yes, even if only two) "icons".

I ended up coloring the icon instead of adding a color dot to the button. I think that avoids the button having too many (yes, even if only two) "icons".
Member

I see.
Sorry, I think I miscommunicated here a bit:

What I meant was that I think we should try to align the title bar of the task detail
with how the task looks in most views. Meaning I think the color dot should prefix the title in order to be aligned.

Since the original issue was about that button not indicating its state:
How about simply replacing the icon with the dot when a color is set? This way we don't have two icons and show the current state at the same time.

I see. Sorry, I think I miscommunicated here a bit: What I meant was that I think we should try to align the title bar of the task detail with how the task looks in most views. Meaning I think the color dot should prefix the title in order to be aligned. Since the original issue was about that button not indicating its state: How about simply replacing the icon with the dot when a color is set? This way we don't have two icons and show the current state at the same time.
Author
Owner

But right now the task color is not shown anywhere but the kanban card. Should we change that so that the dot is visible?

The problem with the dot is we're using it already to show the list color in filters or overviews. Might get difficult to know which one is which.

How about simply replacing the icon with the dot when a color is set? This way we don't have two icons and show the current state at the same time.

If we'll show the color dot I'd do that but right now I like the coloring the icon approach more.

But right now the task color is not shown anywhere but the kanban card. Should we change that so that the dot is visible? The problem with the dot is we're using it already to show the list color in filters or overviews. Might get difficult to know which one is which. > How about simply replacing the icon with the dot when a color is set? This way we don't have two icons and show the current state at the same time. If we'll show the color dot I'd do that but right now I like the coloring the icon approach more.
Member

Should we change that so that the dot is visible?

Since the color is a property of the task I think it makes sense to indicate it in kanban as well.

For me the color dot is just an indicator of the color of the entry it is shown on.
So it would work for namespaces, lists, or tasks in the same way.

> Should we change that so that the dot is visible? Since the color is a property of the task I think it makes sense to indicate it in kanban as well. For me the color dot is just an indicator of the color of the entry it is shown on. So it would work for namespaces, lists, or tasks in the same way.
Author
Owner

Since the color is a property of the task I think it makes sense to indicate it in kanban as well.

Right now the kanban card is the only place where the task color is shown? Do you mean it the other way around?

> Since the color is a property of the task I think it makes sense to indicate it in kanban as well. Right now the kanban card is the only place where the task color is shown? Do you mean it the other way around?
Member

Right now the kanban card is the only place where the task color is shown? Do you mean it the other way around?

Yes, that's what I meant :)

> Right now the kanban card is the only place where the task color is shown? Do you mean it the other way around? Yes, that's what I meant :)
konrad added 1 commit 2022-09-15 11:56:19 +00:00
continuous-integration/drone/pr Build is passing Details
2683fec0a6
feat: show the task color bubble everywhere
Author
Owner

With 2683fec0a6 the color bubble is now visible everywhere when the task has a color.

With https://kolaente.dev/vikunja/frontend/commit/2683fec0a67f6afd16579bb44a6ceadc0edd565f the color bubble is now visible everywhere when the task has a color.
Member

Very nice :)

Very nice :)
dpschen reviewed 2022-09-15 12:26:25 +00:00
@ -42,6 +49,10 @@ const props = defineProps({
type: [String, Array],
default: '',
},
iconColor: {

What I still don't like here is that it's possible to choose a color that doesn't have any contrast to the button.

While the color dot has the same problem we could fix it there by adding a slim border.
The icon on the other hand has such slim lines that the contrast can get bad very easy.

What I still don't like here is that it's possible to choose a color that doesn't have any contrast to the button. While the color dot has the same problem we could fix it there by adding a slim border. The icon on the other hand has such slim lines that the contrast can get bad very easy.
Author
Owner

So we rather shouldn't set the color for the icon?

So we rather shouldn't set the color for the icon?

That's my personal opinion :)

That's my personal opinion :)
dpschen reviewed 2022-09-15 12:28:11 +00:00
@ -0,0 +8,4 @@
<script lang="ts" setup>
defineProps({
color: {
type: String,

Picky: you could improve types here by using:

import type { Color } from "csstype"
Picky: you could improve types here by using: ``` import type { Color } from "csstype" ```
Author
Owner

Nice one. Done.

Nice one. Done.
konrad marked this conversation as resolved
konrad added 1 commit 2022-09-15 12:32:33 +00:00
continuous-integration/drone/pr Build is passing Details
6d9c4a7aa0
chore: improve types
dpschen requested changes 2022-09-15 12:36:00 +00:00
@ -61,2 +61,2 @@
:style="{ backgroundColor: n.hexColor }"
class="color-bubble"
:color="n.hexColor"
class="mr-1"

See comment in ColorBubble component

See comment in ColorBubble component
konrad marked this conversation as resolved
@ -122,0 +117,4 @@
<ColorBubble
v-if="l.hexColor !== ''"
:color="l.hexColor"
class="mr-1"

See comment in ColorBubble component

See comment in ColorBubble component
konrad marked this conversation as resolved
@ -0,0 +18,4 @@
display: inline-block;
border-radius: 100%;
margin-right: 4px;
height: 10px;

If this margin should really be the same everywhere:

We should build this element so that the bubble has that distance included, meaning e.g. we would need to wrap two elements (the outer has an inner padding, the inner is the actual bubble).

If not:

The margin should be controlled from the outside.

If this margin should __really__ be the same everywhere: We should build this element so that the bubble has that distance included, meaning e.g. we would need to wrap two elements (the outer has an inner padding, the inner is the actual bubble). If not: The margin should be controlled from the outside.
Author
Owner

It's intended to not have any margin - the margin here is a leftover from copying the styles from the theme. I've added the margins everywhere where the ColorBubble component is included and the margins are needed.

It's intended to not have any margin - the margin here is a leftover from copying the styles from the theme. I've added the margins everywhere where the ColorBubble component is included and the margins are needed.
dpschen marked this conversation as resolved
@ -8,2 +6,2 @@
>
</span>
:color="listColor"
class="mr-1"

See comment in ColorBubble component

See comment in ColorBubble component
konrad marked this conversation as resolved
@ -23,0 +23,4 @@
<ColorBubble
v-if="task.hexColor !== ''"
:color="task.getHexColor()"
class="mr-1"

See comment in ColorBubble component

See comment in ColorBubble component
konrad marked this conversation as resolved
konrad added 1 commit 2022-09-15 12:37:11 +00:00
dpschen approved these changes 2022-09-15 12:41:57 +00:00
konrad scheduled this pull request to auto merge when all checks succeed 2022-09-15 12:44:44 +00:00
konrad merged commit f70b1d2902 into main 2022-09-15 12:46:13 +00:00
konrad deleted branch feature/color-task-color-button 2022-09-20 17:37:52 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.