feat: color the task color button when the task has a color set #2331
No reviewers
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
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#2331
Loading…
Reference in New Issue
No description provided.
Delete Branch "feature/color-task-color-button"
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?
Partially releated to #811 (comment)
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!
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?
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).
I think coloring the dot is a good tradeoff.
60a79996f4
to51c806c12b
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 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.
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.
If we'll show the color dot I'd do that but right now I like the coloring the icon approach more.
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.
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 :)
With
2683fec0a6
the color bubble is now visible everywhere when the task has a color.Very nice :)
@ -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.
So we rather shouldn't set the color for the icon?
That's my personal opinion :)
@ -0,0 +8,4 @@
<script lang="ts" setup>
defineProps({
color: {
type: String,
Picky: you could improve types here by using:
Nice one. Done.
@ -61,2 +61,2 @@
:style="{ backgroundColor: n.hexColor }"
class="color-bubble"
:color="n.hexColor"
class="mr-1"
See comment in ColorBubble component
@ -122,0 +117,4 @@
<ColorBubble
v-if="l.hexColor !== ''"
:color="l.hexColor"
class="mr-1"
See comment in ColorBubble component
@ -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.
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.
@ -8,2 +6,2 @@
>
</span>
:color="listColor"
class="mr-1"
See comment in ColorBubble component
@ -23,0 +23,4 @@
<ColorBubble
v-if="task.hexColor !== ''"
:color="task.getHexColor()"
class="mr-1"
See comment in ColorBubble component