Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new delta value-display renderer to the React value-display system, enabling rendering of positive/negative deltas with directional icons for use in data-collection/table contexts.
Changes:
- Introduces
DeltaCellrenderer and registers it under thedeltarenderer type. - Adds Storybook stories and MDX docs for the new delta value type.
- Extends shared Storybook mock data with positive/negative delta examples.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/components/value-display/types/delta/index.tsx | Barrel export for the new delta value-display type. |
| packages/react/src/components/value-display/types/delta/delta.tsx | Implements DeltaCell UI (icon + label with status-based color). |
| packages/react/src/components/value-display/types/delta/stories/delta.stories.tsx | Adds Storybook stories demonstrating positive/negative delta rendering. |
| packages/react/src/components/value-display/types/delta/stories/delta.mdx | Adds MDX documentation for the delta value type. |
| packages/react/src/components/value-display/renderers.tsx | Registers delta: DeltaCell in the renderer map. |
| packages/react/src/components/value-display/stories/shared.tsx | Adds positiveDelta / negativeDelta to Storybook mock item data. |
| import { TagListCell } from "./types/tagList" | ||
| import { TeamCell } from "./types/team" | ||
| import { TextCell } from "./types/text" | ||
| import { DeltaCell } from "./types/delta/index.tsx" |
There was a problem hiding this comment.
The DeltaCell import path is inconsistent with the other value-display type imports in this file (it’s the only one importing an explicit /index.tsx). Prefer importing the folder (./types/delta) like the other renderers to keep import style consistent and avoid coupling to the barrel filename/extension.
| import { DeltaCell } from "./types/delta/index.tsx" | |
| import { DeltaCell } from "./types/delta" |
|
|
||
| return ( | ||
| <div className={cn("flex items-center gap-1", colorClass)}> | ||
| {icon ? <F0Icon icon={icon} /> : null} |
There was a problem hiding this comment.
The arrow icon conveys the delta direction (positive/negative) but currently has no accessible name and isn’t marked decorative. This can result in screen readers missing the direction information (or announcing an unlabeled graphic). Add an aria-label (e.g., based on deltaStatus) or mark the icon aria-hidden and include the direction in accessible text.
| {icon ? <F0Icon icon={icon} /> : null} | |
| {icon ? ( | |
| <F0Icon | |
| icon={icon} | |
| aria-label={args.deltaStatus === "positive" ? "Positive change" : "Negative change"} | |
| /> | |
| ) : null} |
1e181d6 to
769d385
Compare
✅ No New Circular DependenciesNo new circular dependencies detected. Current count: 0 |
🔍 Visual review for your branch is published 🔍Here are the links to: |
769d385 to
9cc732d
Compare
📦 Alpha Package Version PublishedUse Use |
| export const DeltaCell = (args: DeltaCellValue) => { | ||
| const icon = iconMap[args.deltaStatus] | ||
| const colorClass = colorMap[args.deltaStatus] | ||
|
|
||
| return ( |
There was a problem hiding this comment.
This PR introduces a new renderer type ("delta") but there are no unit tests for DeltaCell. Similar value-display renderers like IconCell have tests; adding a small test suite (positive/negative render + icon presence + color class) would help prevent regressions.
| const iconMap: Record<DeltaStatus, IconType | undefined> = { | ||
| positive: ArrowUp, | ||
| negative: ArrowDown, | ||
| } |
There was a problem hiding this comment.
iconMap is typed as Record<DeltaStatus, IconType | undefined>, but DeltaStatus only includes "positive" and "negative" and both entries are always defined. This makes icon non-optional in practice and the conditional rendering branch unnecessary; either remove | undefined or add a status that can map to undefined.
|
|
||
| return ( | ||
| <div className={cn("flex items-center gap-1", colorClass)}> | ||
| {icon ? <F0Icon icon={icon} /> : null} |
There was a problem hiding this comment.
The arrow icon is decorative because the label is always rendered. Consider setting aria-hidden on the F0Icon here (or providing an accessible label) to avoid screen readers announcing an unlabeled SVG.
| {icon ? <F0Icon icon={icon} /> : null} | |
| {icon ? <F0Icon icon={icon} aria-hidden /> : null} |
| return ( | ||
| <div className={cn("flex items-center gap-1", colorClass)}> | ||
| {icon ? <F0Icon icon={icon} /> : null} | ||
| <span className={colorClass}>{args.label}</span> |
There was a problem hiding this comment.
colorClass is applied to the wrapper div and again to the span. Since text color is inherited, the span class is redundant and can be removed to avoid duplication.
| <span className={colorClass}>{args.label}</span> | |
| <span>{args.label}</span> |
Coverage Report for packages/react
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
9cc732d to
cae3bbb
Compare
| import { Meta, StoryObj } from "@storybook/react-vite" | ||
|
|
||
| import { Cell, mockItem } from "../../../__stories__/shared" | ||
|
|
||
| const meta = { | ||
| title: "Value Display/Delta", | ||
| component: Cell, | ||
| parameters: { | ||
| layout: "padded", | ||
| docs: { | ||
| description: { | ||
| component: | ||
| "Renders a single delta value with an icon indicating the direction of change. Used for displaying changes or differences in data collections.", | ||
| }, | ||
| source: { | ||
| code: null, | ||
| }, | ||
| }, | ||
| }, | ||
| } satisfies Meta | ||
|
|
||
| export default meta | ||
| type Story = StoryObj<typeof meta> | ||
|
|
||
| export const DeltaType: Story = { | ||
| args: { | ||
| item: mockItem, | ||
| property: { | ||
| label: "Delta", | ||
| render: (item) => ({ | ||
| type: "delta", | ||
| value: item.positiveDelta, | ||
| }), | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| export const NegativeDeltaType: Story = { | ||
| args: { | ||
| item: mockItem, | ||
| property: { | ||
| label: "Delta", | ||
| render: (item) => ({ | ||
| type: "delta", | ||
| value: item.negativeDelta, | ||
| }), | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Storybook stories for components/renderer types in this repo commonly include a dedicated Snapshot story using withSnapshot({}) (often wrapped with withSkipA11y) for Chromatic visual regression. Adding a Snapshot story for both positive and negative delta variants would help prevent regressions.
There was a problem hiding this comment.
Could we better use TagBalance and rename delta to balance? Just to reuse what we already have
https://ds.factorial.dev/?path=/docs/components-tags-tagbalance--documentation
As an example, you can see how we reused it in DetailsItemsList https://ds.factorial.dev/?path=/docs/components-list-detailsitemslist--documentation
Description
Adds a new delta value-display renderer to the React value-display system, enabling rendering of positive/negative deltas with directional icons for use in data-collection/table contexts.
Screenshots (if applicable)
Link to Figma Design
Implementation details