-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TSK-1324: update kanban layout #3118
Conversation
Signed-off-by: Alexander Platov <sas_lord@mail.ru>
&:hover { | ||
color: var(--theme-caption-color); | ||
background-color: var(--theme-button-hovered); | ||
background-color: var(--theme-kanban-button-hover); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it should know about Kanban?
@@ -11,7 +11,7 @@ | |||
export let object: WithLookup<Doc> | |||
export let full: boolean | |||
export let ckeckFilled: boolean = false | |||
export let kind: 'short' | 'full' | 'list' = 'short' | |||
export let kind: 'short' | 'full' | 'list' | 'kanban' = 'short' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not know about Kanban.
background-color: var(--button-bg-hover); | ||
border-color: var(--button-border-hover); | ||
color: var(--theme-caption-color); | ||
background-color: var(--theme-kanban-button-hover); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not know about Kanban.
if (accentColors[i] === undefined) { | ||
accentColors[i] = { | ||
textColor: 'var(--theme-caption-color)', | ||
backgroundColor: '175, 175, 175' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Themes support for backgroundColor?
<span class="text-base fs-bold overflow-label content-accent-color pointer-events-none"> | ||
{#key lth} | ||
<div | ||
style:--kanban-header-rgb-color={accentColors[index].backgroundColor ?? '175, 175, 175'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it could be undefined? Please use method to return value, do not use defaults in multiple places.
const defaultFill = 'currentColor' | ||
$: fill = value ? getPlatformColor(value.color ?? getColorNumberByText(value.name)) : defaultFill | ||
const dispatchAccentColor = (fill: string) => | ||
dispatch('accent-color', fill !== defaultFill ? hexToRgb(fill) : { r: 127, g: 127, b: 127 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have some default color constant in CSS and not in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The { r, g, b } object is expected to be translated to HLS
@@ -18,7 +18,7 @@ | |||
const dispatch = createEventDispatcher() | |||
|
|||
const dispatchAccentColor = (fill: string) => | |||
dispatch('accent-color', fill !== defaultFill ? hexToRgb(fill) : 'var(--theme-halfcontent-color)') | |||
dispatch('accent-color', fill !== defaultFill ? hexToRgb(fill) : { r: 127, g: 127, b: 127 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default color constant in CSS again.
if (accentColors[i] === undefined) { | ||
accentColors[i] = { | ||
textColor: 'var(--theme-caption-color)', | ||
backgroundColor: '175, 175, 175' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default color constant again.
|
||
const setAccentColor = (n: number, ev: CustomEvent) => { | ||
const accColor = rgbToHsl(ev.detail.r, ev.detail.g, ev.detail.b) | ||
const textColor = !lth ? { r: 255, g: 255, b: 255 } : hslToRgb(accColor.h, accColor.s, 0.3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default color constant again.
const setAccentColor = (n: number, ev: CustomEvent) => { | ||
const accColor = rgbToHsl(ev.detail.r, ev.detail.g, ev.detail.b) | ||
const textColor = !lth ? { r: 255, g: 255, b: 255 } : hslToRgb(accColor.h, accColor.s, 0.3) | ||
const bgColor = !lth ? hslToRgb(accColor.h, accColor.s, 0.55) : hslToRgb(accColor.h, accColor.s, 0.9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move 0.55 and 0.9 into CSS if possible.
<div class="flex-row-center flex-between"> | ||
{#key lth} | ||
<div | ||
style:--kanban-header-rgb-color={accentColors[index].backgroundColor ?? '175, 175, 175'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default color constant again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check review comments.
Contribution checklist
Brief description
Checklist
Related issues
A list of closed updated issues