-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Feature-7111-Unify column width definitions across spreadsheet components #8730
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
base: preview
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import type { IIssueDisplayProperties, TIssue } from "@plane/types"; | ||
|
|
||
| export const COLUMN_WIDTHS: Partial<Record<keyof IIssueDisplayProperties, string>> = { | ||
| state: "80px", | ||
| priority: "80px", | ||
| assignee: "120px", | ||
| estimate: "80px", | ||
| labels: "80px", | ||
| start_date: "120px", | ||
| due_date: "120px", | ||
| created_on: "120px", | ||
| updated_on: "120px", | ||
| link: "80px", | ||
| attachment_count: "80px", | ||
| sub_issue_count: "80px", | ||
| cycle: "120px", | ||
| modules: "120px", | ||
| }; | ||
|
|
||
| export function getColumnWidth(property: keyof IIssueDisplayProperties): string { | ||
| return COLUMN_WIDTHS[property] ?? "auto"; | ||
| } | ||
|
|
||
| export async function handleUpdateIssueLogic( | ||
| updateIssue: ((projectId: string | null, issueId: string, data: Partial<TIssue>) => Promise<void>) | undefined, | ||
| issue: TIssue, | ||
| data: Partial<TIssue> | ||
| ): Promise<void> { | ||
| if (!updateIssue) return; | ||
| await updateIssue(issue.project_id, issue.id, data); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import type { IIssueDisplayProperties } from "@plane/types"; | ||
|
|
||
| export const HEADER_COLUMN_WIDTHS: Partial<Record<keyof IIssueDisplayProperties, string>> = { | ||
| state: "80px", | ||
| priority: "80px", | ||
| assignee: "120px", | ||
| estimate: "80px", | ||
| labels: "80px", | ||
| start_date: "120px", | ||
| due_date: "120px", | ||
| created_on: "120px", | ||
| updated_on: "120px", | ||
| link: "80px", | ||
| attachment_count: "80px", | ||
| sub_issue_count: "80px", | ||
| cycle: "120px", | ||
| modules: "120px", | ||
| }; | ||
|
|
||
| export function getHeaderColumnWidth(property: keyof IIssueDisplayProperties): string { | ||
| return HEADER_COLUMN_WIDTHS[property] ?? "auto"; | ||
| } | ||
|
Comment on lines
+3
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep a single source of truth for column widths.
🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import type { IIssueDisplayFilterOptions, IIssueDisplayProperties } from "@plane | |
| import { shouldRenderColumn } from "@/plane-web/helpers/issue-filter.helper"; | ||
| import { WithDisplayPropertiesHOC } from "../properties/with-display-properties-HOC"; | ||
| import { HeaderColumn } from "./columns/header-column"; | ||
| import { getHeaderColumnWidth } from "./spreadsheet-header-column.logic"; | ||
|
|
||
| interface Props { | ||
| displayProperties: IIssueDisplayProperties; | ||
|
|
@@ -37,6 +38,7 @@ export const SpreadsheetHeaderColumn = observer(function SpreadsheetHeaderColumn | |
| > | ||
| <th | ||
| className="h-11 min-w-36 items-center bg-layer-1 text-13 font-medium py-1 border border-b-0 border-t-0 border-subtle" | ||
| style={{ width: getHeaderColumnWidth(property) }} | ||
| ref={tableHeaderCellRef} | ||
|
Comment on lines
39
to
42
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Line 40 still forces every header cell to at least 🤖 Prompt for AI Agents |
||
| tabIndex={0} | ||
| > | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { describe, it, expect, vi } from "vitest"; | ||
| import { | ||
| getColumnWidth, | ||
| handleUpdateIssueLogic, | ||
| COLUMN_WIDTHS, | ||
| } from "../../../apps/web/core/components/issues/issue-layouts/spreadsheet/issue-column.logic"; | ||
| import type { TIssue } from "@plane/types"; | ||
|
|
||
| describe("issue-column.logic", () => { | ||
| describe("getColumnWidth", () => { | ||
| it("returns the correct width for known properties", () => { | ||
| for (const key of Object.keys(COLUMN_WIDTHS) as Array< | ||
| keyof typeof COLUMN_WIDTHS | ||
| >) { | ||
| expect(getColumnWidth(key)).toBe(COLUMN_WIDTHS[key]); | ||
| } | ||
| }); | ||
|
|
||
| it("returns auto for unknown properties", () => { | ||
| // @ts-expect-error testing fallback | ||
| expect(getColumnWidth("unknown")).toBe("auto"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("handleUpdateIssueLogic", () => { | ||
| const issue = { | ||
| id: "123", | ||
| project_id: "p1", | ||
| } as unknown as TIssue; | ||
|
|
||
| it("does nothing when updateIssue is undefined", async () => { | ||
| const result = await handleUpdateIssueLogic(undefined, issue, { | ||
| name: "x", | ||
| }); | ||
| expect(result).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("calls updateIssue with correct arguments", async () => { | ||
| const mock = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| await handleUpdateIssueLogic(mock, issue, { name: "New Name" }); | ||
|
|
||
| expect(mock).toHaveBeenCalledWith("p1", "123", { name: "New Name" }); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
| import { | ||
| getHeaderColumnWidth, | ||
| HEADER_COLUMN_WIDTHS, | ||
| } from "../../../apps/web/core/components/issues/issue-layouts/spreadsheet/spreadsheet-header-column.logic"; | ||
|
|
||
| describe("spreadsheet-header-column.logic", () => { | ||
| describe("getHeaderColumnWidth", () => { | ||
| it("returns correct width for known properties", () => { | ||
| for (const key of Object.keys(HEADER_COLUMN_WIDTHS) as Array< | ||
| keyof typeof HEADER_COLUMN_WIDTHS | ||
| >) { | ||
| expect(getHeaderColumnWidth(key)).toBe(HEADER_COLUMN_WIDTHS[key]); | ||
| } | ||
| }); | ||
|
|
||
| it("returns auto for unknown properties", () => { | ||
| // @ts-expect-error testing fallback | ||
| expect(getHeaderColumnWidth("unknown")).toBe("auto"); | ||
| }); | ||
| }); | ||
| }); |
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.
Don’t fire-and-forget the issue update promise.
Line 55 drops rejections from the async update path, so a failed write can turn into an unhandled promise rejection with no user-visible recovery. Please catch/log the error here, or make the callback async and handle failures explicitly. As per coding guidelines, "Use try-catch with proper error types and log errors appropriately for error handling".
🤖 Prompt for AI Agents