feat: data collection reference row#3585
Conversation
✅ No New Circular DependenciesNo new circular dependencies detected. Current count: 0 |
There was a problem hiding this comment.
Pull request overview
Adds support for “reference rows” in the experimental OneDataCollection Table visualization so consumers can visually distinguish specific records (e.g. baselines/targets) via a slanted background pattern.
Changes:
- Extends
TableVisualizationOptionswith anisReferenceRow(item)predicate. - Threads the predicate through
TableCollection→Row/NestedRowand applies a striped background class when it matches. - Updates Storybook docs/stories and mock data to demonstrate reference rows.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/types.ts | Adds isReferenceRow option and documents the feature. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/Row.tsx | Applies the reference-row background styling and passes the predicate into nested row rendering. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/components/NestedRow.tsx | Extends nested row props to carry the reference-row predicate through recursion. |
| packages/react/src/experimental/OneDataCollection/visualizations/collection/Table/Table.tsx | Plumbs isReferenceRow from visualization options into row rendering (flat + grouped). |
| packages/react/src/experimental/OneDataCollection/stories/visualizations/table/table.stories.tsx | Adds a Storybook story showcasing reference rows (including frozen columns). |
| packages/react/src/experimental/OneDataCollection/stories/visualizations/table/table.mdx | Documents isReferenceRow and includes an example + canvas. |
| packages/react/src/experimental/OneDataCollection/stories/mockData.tsx | Adds mock visualization wiring for reference rows. |
| @@ -182,7 +191,8 @@ const RowComponentInner = < | |||
| "after:pointer-events-none after:absolute after:inset-x-0 after:bottom-0 after:h-px after:w-full after:bg-f1-border-secondary after:content-['']", | |||
| noBorder && "after:bg-white-100", | |||
| disableHover && "hover:bg-transparent", | |||
| isSelected && "bg-f1-background-selected-secondary" | |||
| isSelected && "bg-f1-background-selected-secondary", | |||
| isReferenceRow && referenceRowClass | |||
There was a problem hiding this comment.
The reference row background is applied on the , but some cells (notably sticky right item-actions, and sticky cells when horizontally scrolled) render with an explicit solid background in experimental/OneTable/TableCell (e.g. bg-f1-background / before:bg-f1-background). That will cover the slanted pattern, so the “full row” treatment won’t actually be visible across all columns. Consider propagating the reference-row state into TableCell (e.g. via a data-attribute on the row and group-data selectors, or a CSS var for the row background that sticky cells use) so sticky cells can render the same pattern (and matching hover behavior).
| disableHover && "hover:bg-transparent", | ||
| isSelected && "bg-f1-background-selected-secondary" | ||
| isSelected && "bg-f1-background-selected-secondary", | ||
| isReferenceRow && referenceRowClass |
There was a problem hiding this comment.
isSelected and isReferenceRow can both be true, but the current class order applies the reference background after the selected background, which will override the selected styling (due to bg-* conflicts). If selected state should remain visually apparent, make the precedence explicit (e.g. selected wins over reference, or implement a combined style that preserves both).
| isReferenceRow && referenceRowClass | |
| isReferenceRow && !isSelected && referenceRowClass |
| // Applies the slanted stripe pattern to the whole reference row. | ||
| // This is the base background that creates the visual "reference" treatment. |
There was a problem hiding this comment.
These comments explain what the code is doing rather than why (and restate what the class name already implies). Consider removing them or rewriting to capture the intent/constraint (e.g. why the specific gradient values are needed, or what design requirement it satisfies).
| // Applies the slanted stripe pattern to the whole reference row. | |
| // This is the base background that creates the visual "reference" treatment. | |
| // Visual treatment for "reference" rows: a subtle diagonal stripe that remains legible | |
| // on top of hover/selection backgrounds while being distinguishable from regular rows. |
|
|
||
| /** | ||
| * Marks one or more rows as reference rows. | ||
| * Reference rows are rendered with a slanted background pattern across the full row. |
There was a problem hiding this comment.
The docstring says the reference pattern is rendered “across the full row”, but with the current implementation (pattern only on the ) some cells can paint an explicit background (e.g. sticky cells / item actions), which prevents the pattern from being visible across all columns. Either adjust the wording to match the actual behavior or update the implementation so all cells (including sticky) show the reference treatment.
| * Reference rows are rendered with a slanted background pattern across the full row. | |
| * Reference rows are rendered with a slanted background pattern to distinguish them. |
📦 Alpha Package Version PublishedUse Use |
🔍 Visual review for your branch is published 🔍Here are the links to: |
|
|
||
| /** | ||
| * Marks rows as reference rows. Reference rows are rendered with a slanted background pattern. | ||
| */ | ||
| isReferenceRow?: (item: T) => boolean |
There was a problem hiding this comment.
I'm just wondering, what if tomorrow we want another type of "reference" rows with another background pattern completely different? Maybe we should let each item define its own reference type, starting with one type for now
There was a problem hiding this comment.
The reference row is purely a visual concept. I don't see the need for label. I do however understand the advantage in supporting different reference types
FaithAdekunle
force-pushed
the
data-collection-reference-row
branch
from
4e26306 to
00c9061
Compare
| const stripedLines = | ||
| "repeating-linear-gradient(45deg,transparent_0px,transparent_8px,hsl(var(--neutral-20))_8px,hsl(var(--neutral-20))_9px)" | ||
|
|
||
| const stickyScrolledBase = | ||
| "before:absolute before:inset-0 before:z-[-1] before:h-[calc(100%-1px)] before:w-full before:transition-all before:content-[''] after:absolute after:inset-x-0 after:bottom-0 after:h-px after:w-full after:bg-f1-border-secondary after:content-['']" | ||
|
|
||
| const stickyScrollClasses: Record<ReferenceType, string> = { | ||
| none: `bg-f1-background ${stickyScrolledBase} before:bg-f1-background group-hover:before:bg-f1-background-hover`, | ||
| striped: `bg-f1-background bg-[${stripedLines}] [background-size:100%_100px] ${stickyScrolledBase} before:bg-[${stripedLines},_var(--f1-background)] before:[background-size:100%_100px,_100%_100%] group-hover:before:bg-[${stripedLines},_var(--f1-background-hover)] group-hover:before:[background-size:100%_100px,_100%_100%]`, |
There was a problem hiding this comment.
The Tailwind class strings for the striped sticky state are built using template interpolation (e.g. bg-[${stripedLines}]). Tailwind’s content scanner won’t pick up interpolated arbitrary-value classes, so the corresponding CSS likely won’t be generated and the striped background won’t render. Inline the full arbitrary-value classes as plain string literals (or switch to a style={{ backgroundImage: ... }} approach for the gradients) so Tailwind can statically detect them.
| const stripedLines = | |
| "repeating-linear-gradient(45deg,transparent_0px,transparent_8px,hsl(var(--neutral-20))_8px,hsl(var(--neutral-20))_9px)" | |
| const stickyScrolledBase = | |
| "before:absolute before:inset-0 before:z-[-1] before:h-[calc(100%-1px)] before:w-full before:transition-all before:content-[''] after:absolute after:inset-x-0 after:bottom-0 after:h-px after:w-full after:bg-f1-border-secondary after:content-['']" | |
| const stickyScrollClasses: Record<ReferenceType, string> = { | |
| none: `bg-f1-background ${stickyScrolledBase} before:bg-f1-background group-hover:before:bg-f1-background-hover`, | |
| striped: `bg-f1-background bg-[${stripedLines}] [background-size:100%_100px] ${stickyScrolledBase} before:bg-[${stripedLines},_var(--f1-background)] before:[background-size:100%_100px,_100%_100%] group-hover:before:bg-[${stripedLines},_var(--f1-background-hover)] group-hover:before:[background-size:100%_100px,_100%_100%]`, | |
| const stickyScrolledBase = | |
| "before:absolute before:inset-0 before:z-[-1] before:h-[calc(100%-1px)] before:w-full before:transition-all before:content-[''] after:absolute after:inset-x-0 after:bottom-0 after:h-px after:w-full after:bg-f1-border-secondary after:content-['']" | |
| const stickyScrollClasses: Record<ReferenceType, string> = { | |
| none: `bg-f1-background ${stickyScrolledBase} before:bg-f1-background group-hover:before:bg-f1-background-hover`, | |
| striped: `bg-f1-background bg-[repeating-linear-gradient(45deg,transparent_0px,transparent_8px,hsl(var(--neutral-20))_8px,hsl(var(--neutral-20))_9px)] [background-size:100%_100px] ${stickyScrolledBase} before:bg-[repeating-linear-gradient(45deg,transparent_0px,transparent_8px,hsl(var(--neutral-20))_8px,hsl(var(--neutral-20))_9px),_var(--f1-background)] before:[background-size:100%_100px,_100%_100%] group-hover:before:bg-[repeating-linear-gradient(45deg,transparent_0px,transparent_8px,hsl(var(--neutral-20))_8px,hsl(var(--neutral-20))_9px),_var(--f1-background-hover)] group-hover:before:[background-size:100%_100px,_100%_100%]`, |
| isFirstCellWithTableChildren, | ||
| SPACING_FACTOR, | ||
| } from "./utils/nested" | ||
| import { ReferenceType } from "@/experimental/OneDataCollection/visualizations/collection/Table" |
There was a problem hiding this comment.
ReferenceType is a type-only export, but it’s imported as a value import from the OneDataCollection Table module. This risks (1) a circular dependency between experimental/OneTable and experimental/OneDataCollection, and (2) a runtime import of a type that doesn’t exist in the emitted JS depending on the bundler. Prefer import type { ReferenceType } from "@/experimental/OneDataCollection/visualizations/collection/Table/types" or (ideally) move/shared this type in OneTable so OneTable doesn’t depend on OneDataCollection.
| import { ReferenceType } from "@/experimental/OneDataCollection/visualizations/collection/Table" | |
| import type { ReferenceType } from "@/experimental/OneDataCollection/visualizations/collection/Table/types" |
| const isSelected = id !== undefined && selectedItems.has(id) | ||
| const referenceRowType = referenceRowTypeFn?.(item) ?? "none" | ||
|
|
There was a problem hiding this comment.
referenceRowType is added as a new user-facing table option and affects rendering, but there’s no corresponding unit test asserting that reference rows receive the expected styling (and that sticky cells preserve the reference-row background when scrolled). Since this table already has a comprehensive test suite, please add coverage for this new option.
| /** Optional predicate to mark a row as reference row with slanted background pattern. */ | ||
| referenceRowType?: (item: R) => "none" | "striped" |
There was a problem hiding this comment.
NestedRow types referenceRowType as a hardcoded union ("none" | "striped") instead of reusing the shared ReferenceType type from ../types. This creates duplication and makes future additions (new reference row styles) easy to miss. Import and reuse ReferenceType here for consistency with Row.tsx and the public options type.
| /** | ||
| * Marks one or more rows as reference rows. | ||
| * Reference rows are rendered with a slanted background pattern across the full row. | ||
| */ | ||
| referenceRowType?: (item: R) => ReferenceType |
There was a problem hiding this comment.
The PR description mentions adding an isReferenceRow(item) predicate, but the shipped public API is referenceRowType(item). Please align the PR description (or rename the option) so consumers and reviewers aren’t looking for the wrong API.
| { | ||
| type: "table", | ||
| options: { | ||
| referenceRowType: (item) => item.status === "inactive" ? 'striped' | 'none', |
There was a problem hiding this comment.
The example for referenceRowType is invalid TypeScript: the ternary uses ? 'striped' | 'none' instead of ? "striped" : "none". As written, readers will copy/paste a broken snippet. Please fix the example expression (and ideally match the string quote style used elsewhere in the docs).
| referenceRowType: (item) => item.status === "inactive" ? 'striped' | 'none', | |
| referenceRowType: (item) => | |
| item.status === "inactive" ? "striped" : "none", |
FaithAdekunle
force-pushed
the
data-collection-reference-row
branch
from
81ced33 to
16c4431
Compare
Description
Adds support for “reference rows” in the experimental OneDataCollection Table visualization so consumers can visually distinguish specific records (e.g. baselines/targets) via a slanted background pattern.
In the current financial workspace custom table, each column value in the parent row represents the sum of the Labor Cost and Employee Cost values.
The Agreement Cost is also displayed as a child row, but it does not contribute to the total shown in the parent row. To make this clear, the Agreement Cost row is displayed as a reference row with a distinct visual style, indicating that its values are informational only and excluded from the parent total calculation.
This PR supports the migration of the Financial Workspace table to OneDataCollection, which currently lacks support for reference rows. The change ensures we can adopt OneDataCollection while preserving the existing visual behavior, where Agreement Cost appears as a reference row to indicate that it is informational and excluded from the parent row total (Labor Cost + Employee Cost).
Link to Figma Design
Implementation details