feat(ui): tile version of data lists #406
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
WalkthroughA pull request introducing a grid view mode to the ticket list block with a new DataGrid component, FilterViewModeToggle filter type, and extended TicketListBlock model with slots configuration and viewMode filtering across multiple packages. Changes
Sequence DiagramsequenceDiagram
participant User
participant TicketList as TicketList Component
participant FiltersSection as Filters Section
participant ViewState as View Mode State
participant DataGrid
participant DataList
User->>TicketList: Load with data & filters
TicketList->>ViewState: Extract viewMode from filters (default: list)
TicketList->>FiltersSection: Pass filters with viewMode
User->>FiltersSection: Toggle view mode (grid/list)
FiltersSection->>FiltersSection: FilterViewModeToggle onChange
FiltersSection->>TicketList: onSubmit(filters)
TicketList->>ViewState: Update viewMode
alt viewMode === 'grid'
TicketList->>DataGrid: Render with data + slots
DataGrid->>DataGrid: Render responsive grid<br/>(top/left/right/bottom slots)
else viewMode === 'list'
TicketList->>DataList: Render with data
DataList->>DataList: Render table rows<br/>(via renderCell utility)
end
DataGrid-->>User: Grid view
DataList-->>User: List view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/blocks/ticket-list/src/frontend/TicketList.client.stories.tsx (1)
58-62: Story slots also pointleftto non‑existentidcolumnThis story uses the same
slotsconfig as the mocks (left: 'id'), but the table columns still don’t include anidcolumn, so the left slot won’t render anything in the grid header. This is the same issue noted in the CMS ticket‑list mocks; once you adjust the slots/columns there, please mirror the fix here to keep the story in sync.
🧹 Nitpick comments (9)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (1)
27-32: Slots API shape looks good and stays backward compatibleThe optional
slotsobject mirrors the CMS model and cleanly extendsTicketListBlockwithout breaking existing consumers. If you find yourself reusing this shape elsewhere, consider extracting a sharedSlotsConfigtype to avoid duplication, but it’s not urgent.packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts (1)
146-150: View‑mode toggle mocks are consistent across localesThe added
FilterViewModeToggleitems withid: 'viewMode'and defaultvalue: 'grid'are consistent in EN/DE/PL and line up with the new filter type. If you want the legacy list view to remain the default, you might switch these defaults to'list', but as mocks they’re fine either way.Also applies to: 311-315, 477-481
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
39-46: View‑mode state is independent of filter reset; confirm intended UX
initialViewModeis derived from the incoming filters once and stored in local state:const initialViewMode = component.filters?.items.find((item) => item.__typename === 'FilterViewModeToggle')?.value || 'list'; const [viewMode, setViewMode] = useState<'list' | 'grid'>(initialViewMode);
handleResetresets backend filters/data but doesn’t resetviewMode, andFilterViewModeToggleis wired only viaonChange: setViewMode. That means “Reset/Clear filters” keeps the current view mode, effectively treating it as a persistent UI preference rather than a filter.If that’s the desired behavior, this is fine. If you expect reset to also revert the view mode to its initial value, you could additionally call
setViewMode(initialViewMode)insidehandleReset.Also applies to: 58-64, 167-183
packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts (1)
12-30: Filters/viewMode and slots typing look consistent; consider centralizing the slots typeThe added
viewModein the filters type matches the new toggle and should integrate cleanly with the form model, and theslotsshape is aligned with the DataGrid/DataList usage. As a small cleanup, you might want to extract a sharedSlotsConfigtype (e.g. in a common models/util file) so the{ top; left; right; bottom }structure is not duplicated between CMS models and UI props.packages/framework/src/utils/models/filters.ts (1)
12-17: MakeFilterViewModeToggle.onChangerequired to match UI usage
FilterViewModeToggle.onChangeis optional here, but the UI renderer (FilterItemcase 'FilterViewModeToggle') callsitem.onChange!unconditionally. This makes it easy to introduce a runtime error if a caller forgets to wireonChange. Making it required keeps the types honest and prevents silent misconfiguration.-export class FilterViewModeToggle { +export class FilterViewModeToggle { __typename!: 'FilterViewModeToggle'; id!: 'viewMode'; - value?: 'list' | 'grid'; - onChange?: (value: 'list' | 'grid') => void; + value?: 'list' | 'grid'; + onChange!: (value: 'list' | 'grid') => void; }Also applies to: 61-66
packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
29-53: Tightenslotstyping to column IDs for better safetyThe props shape looks good and matches the component, but
slotsbeing plainstringloses type-safety w.r.t. the configured columns. You can make slot ids line up withDataListColumnConfig<T>['id']so mismatched slot names are caught at compile time:-export interface DataGridProps<T> { +export interface DataGridProps<T> { @@ - slots?: { - top?: string; - left?: string; - right?: string; - bottom?: string; - }; + slots?: { + top?: DataListColumnConfig<T>['id']; + left?: DataListColumnConfig<T>['id']; + right?: DataListColumnConfig<T>['id']; + bottom?: DataListColumnConfig<T>['id']; + };Optionally, you could also align with the component by constraining
There (DataGridProps<T extends Record<string, any>>) so callers get consistent expectations aboutT.packages/ui/src/lib/renderCell.tsx (1)
16-69: Loosenvaluetyping and guard badge access to avoid unnecessary casts
renderCellcurrently requiresvalue: Record<string, unknown>, but many callers pass primitives and have to cast arbitrarily (as Record<string, unknown>). You’re already checkingtypeof value === 'object'in most cases; you could reflect that in the type and slightly harden the badge branch:-export function renderCell<T>( - value: Record<string, unknown>, +export function renderCell<T>( + // Allow primitives; branches already narrow as needed + value: unknown, @@ - case 'badge': { - const badgeLabel = String(value[column.labelField || 'label']); - const badgeValue = String(value[column.valueField || 'value']); + case 'badge': { + if (typeof value !== 'object' || value === null) { + return null; + } + const record = value as Record<string, unknown>; + const badgeLabel = String(record[column.labelField || 'label']); + const badgeValue = String(record[column.valueField || 'value']); const variant = column.variant ? column.variant(badgeValue) : 'default';This makes the signature better match actual usage and avoids unsafe indexing when a badge cell value isn’t an object.
packages/ui/src/components/DataGrid/DataGrid.stories.tsx (1)
148-261: Tighten story typings and de‑duplicate common argsThe stories are fine functionally, but the repeated
as Record<string, unknown>[]/as DataListColumnConfig<Record<string, unknown>>[]casts weaken type‑safety and duplicate the samedata/columns/slotssetup in several places.If you want stronger typing with minimal change, consider:
- Typing
argsasDataGridProps<Ticket>directly (e.g. via a sharedconst baseArgs: DataGridProps<Ticket> = { … }) and- Reusing
baseArgswith spreads in individual stories to reduce duplication.This keeps the stories easier to maintain and lets TS catch drift between
TicketandticketColumns.packages/ui/src/components/Filters/Filters.tsx (1)
17-33:separateLeadingItemcorrectly isolates the view mode toggleThe updated
separateLeadingItemcleanly separates:
- a single
leadingItem,- a single
FilterViewModeToggle, and- the remaining
filteredItems.This matches the intended behavior where view mode is orthogonal to form‑driven filters. It assumes there is at most one
FilterViewModeToggle, which is reasonable for current usage; if that ever changes, you might want to assert or log when multiple toggles are passed instead of silently taking the last one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts(1 hunks)packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts(1 hunks)packages/blocks/ticket-list/src/frontend/TicketList.client.stories.tsx(2 hunks)packages/blocks/ticket-list/src/frontend/TicketList.client.tsx(5 hunks)packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts(2 hunks)packages/framework/src/utils/models/filters.ts(2 hunks)packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts(6 hunks)packages/ui/src/components/DataGrid/DataGrid.stories.tsx(1 hunks)packages/ui/src/components/DataGrid/DataGrid.tsx(1 hunks)packages/ui/src/components/DataGrid/DataGrid.types.ts(1 hunks)packages/ui/src/components/DataGrid/index.ts(1 hunks)packages/ui/src/components/DataList/DataList.tsx(1 hunks)packages/ui/src/components/Filters/FilterItem.tsx(2 hunks)packages/ui/src/components/Filters/Filters.tsx(3 hunks)packages/ui/src/components/Filters/FiltersSection.tsx(1 hunks)packages/ui/src/lib/renderCell.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
packages/ui/src/lib/renderCell.tsx (2)
packages/framework/src/utils/models/badge.ts (1)
Badge(1-4)packages/framework/src/utils/models/price.ts (1)
Price(3-7)
packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts (3)
packages/framework/src/utils/models/filters.ts (1)
Filters(1-10)packages/ui/src/components/Filters/Filters.tsx (1)
Filters(35-160)packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (1)
Ticket(35-52)
packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
packages/ui/src/components/DataGrid/index.ts (2)
DataGridColumnsCount(2-2)DataGridProps(2-2)
packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (4)
packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (2)
TicketListBlock(7-33)Ticket(35-52)packages/framework/src/modules/cms/models/blocks/ticket-list.model.ts (1)
TicketListBlock(6-31)packages/ui/src/components/DataGrid/DataGrid.tsx (1)
DataGrid(19-194)packages/ui/src/components/DataList/DataList.tsx (1)
DataList(13-88)
packages/ui/src/components/Filters/FilterItem.tsx (1)
packages/ui/src/elements/toggle-group.tsx (2)
ToggleGroup(70-70)ToggleGroupItem(70-70)
packages/ui/src/components/DataGrid/DataGrid.stories.tsx (3)
packages/ui/src/components/DataGrid/DataGrid.tsx (1)
DataGrid(19-194)packages/blocks/ticket-list/src/api-harmonization/ticket-list.model.ts (1)
Ticket(35-52)packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
DataGridProps(29-74)
packages/ui/src/components/DataGrid/DataGrid.tsx (3)
packages/ui/src/components/DataGrid/DataGrid.types.ts (1)
DataGridProps(29-74)packages/ui/src/lib/utils.ts (1)
cn(5-7)packages/ui/src/lib/renderCell.tsx (1)
renderCell(16-100)
packages/ui/src/components/Filters/Filters.tsx (2)
packages/framework/src/utils/models/filters.ts (2)
FilterViewModeToggle(61-66)FilterItem(12-17)packages/ui/src/components/Filters/FilterItem.tsx (1)
FilterItem(16-227)
packages/framework/src/utils/models/filters.ts (1)
packages/ui/src/components/Filters/FilterItem.tsx (1)
FilterItem(16-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy-preview
🔇 Additional comments (8)
packages/ui/src/components/Filters/FiltersSection.tsx (1)
23-23: Robust leading‑item check for mixed filter item typesUsing
'isLeading' in item && item.isLeading === trueis a good improvement: it avoids relying on properties that may not exist on all filter item variants and keeps layout logic from being triggered by items that don’t supportisLeading.packages/blocks/ticket-list/src/frontend/TicketList.client.stories.tsx (1)
216-220: Storybook wiring forFilterViewModeTogglelooks goodIncluding a
FilterViewModeTogglewithvalue: 'grid'ensures the new grid view is exercised in Storybook and makes it easy to validate the toggle UI.packages/blocks/ticket-list/src/api-harmonization/ticket-list.mapper.ts (1)
29-29: Slots passthrough keeps API harmonization aligned with CMSMapping
slots: cms.slotsdirectly into theTicketListBlockoutput cleanly exposes the CMS configuration to the frontend and matches the updated model type. No issues here.packages/ui/src/components/DataGrid/index.ts (1)
1-2: Public DataGrid exports are straightforward and consistentRe‑exporting
DataGridplus its props andDataGridColumnsCounttype from an index barrel is aligned with the rest of the UI components and keeps the import surface tidy.packages/blocks/ticket-list/src/frontend/TicketList.client.tsx (1)
97-120: Grid/list‑aware actions and body rendering look consistentOverriding
actions.renderto switch button variant/layout based onviewMode, and conditionally rendering eitherDataGrid(withslots={data.slots}) orDataListusing the samecolumnsconfig, keeps behavior consistent between views while adapting the UI affordances appropriately. No issues spotted here.Also applies to: 195-204
packages/ui/src/components/DataGrid/DataGrid.stories.tsx (1)
23-146: Sample Ticket model and column config are coherent with DataGrid expectationsThe
Ticketshape,sampleTickets, andticketColumnsline up well with the DataGrid + sharedrenderCellmodel (label/value objects for text/date fields and astatusbadge variant based on the value). This should give very representative stories for ticket‑list usage.packages/ui/src/components/DataList/DataList.tsx (1)
3-87: Refactor to sharedrenderCelland table primitives looks solidDelegating cell rendering to
renderCelland using the localTableprimitives keeps DataList lean while preserving behavior (row keys, header rendering, per‑column cellClassName, actions column, and text truncation). I don’t see regressions here; this should make it much easier to keep DataList/DataGrid rendering consistent.packages/ui/src/components/Filters/Filters.tsx (1)
51-96: Rendering leading filter and view mode toggle side‑by‑side is wired correctlyThe top bar now:
- Keeps the leading filter inside the horizontal
ScrollContainer, and- Places the
viewModeToggleimmediately next to it as a regularFilterItem.
viewModeToggleis excluded from the sheet’sfilteredItemslist, so it only appears once, and it doesn’t interfere with Formik state (consistent withFilterItem’s standalone handling forFilterViewModeToggle). The flex layout and keys all look correct.
| slots: { | ||
| left: 'id', | ||
| right: 'status', | ||
| bottom: 'updatedAt', | ||
| }, |
There was a problem hiding this comment.
slots.left = 'id' doesn’t match any table column ID
In all three mocks, slots.left is set to 'id', but the table.columns arrays only define topic, type, status, and updatedAt. Since DataGrid looks up slot columns by columns.find((col) => col.id === slotId), the left slot won’t resolve and the ticket ID won’t render in the card header.
Consider either:
- Adding an
idcolumn totable.columns(and handling it appropriately in list view), or - Adjusting the grid implementation/slots semantics to allow using raw fields like
idthat aren’t part oftable.columns.
Also applies to: 199-203, 365-369
🤖 Prompt for AI Agents
packages/integrations/mocked/src/modules/cms/mappers/blocks/cms.ticket-list.mapper.ts
lines 36-40 (also applies to 199-203 and 365-369): the left slot is set to 'id'
but the table.columns arrays do not include a column with id 'id', so DataGrid
won't resolve the slot and the ticket ID won't render; fix by either adding a
column entry with id: 'id' to each table.columns array (and ensure the list view
rendering handles that column appropriately, e.g., hidden in grid but used for
header) or change the slot value to an existing column id (for example 'topic'
or another appropriate column), or modify the grid lookup to fall back to raw
fields when columns.find(...) returns undefined—pick one approach and apply
consistently to all three mock blocks.
| return ( | ||
| <div className={gridClasses}> | ||
| {data.map((item, index) => { | ||
| const rowKey = rowKeyExtractor(item, index); | ||
| const rowClassName = getRowClassName ? getRowClassName(item) : undefined; | ||
| const bodyColumns = getBodyColumns(); | ||
|
|
||
| // Split body columns into two groups for two-column layout | ||
| const midPoint = Math.ceil(bodyColumns.length / 2); | ||
| const leftColumns = bodyColumns.slice(0, midPoint); | ||
| const rightColumns = bodyColumns.slice(midPoint); | ||
|
|
||
| return ( | ||
| <Card key={rowKey} className={cn('flex flex-col', rowClassName)}> | ||
| {(topColumn || leftColumn || rightColumn || bottomColumn) && ( | ||
| <CardHeader className="pb-3"> | ||
| {topColumn && ( | ||
| <div className="mb-2"> | ||
| {renderCell(item[topColumn.id] as Record<string, unknown>, item, topColumn)} | ||
| </div> | ||
| )} | ||
| <div className="flex items-start justify-between gap-4"> | ||
| {(leftColumn || bottomColumn) && ( | ||
| <div className="flex-1 flex flex-col gap-2"> | ||
| {leftColumn && ( | ||
| <Typography variant="h3" className="font-bold"> | ||
| {renderCell( | ||
| item[leftColumn.id] as Record<string, unknown>, | ||
| item, | ||
| leftColumn, | ||
| )} | ||
| </Typography> | ||
| )} | ||
| {bottomColumn && ( | ||
| <Typography variant="small" className="text-muted-foreground"> | ||
| {renderCell( | ||
| item[bottomColumn.id] as Record<string, unknown>, | ||
| item, | ||
| bottomColumn, | ||
| )} | ||
| </Typography> | ||
| )} | ||
| </div> | ||
| )} | ||
| {rightColumn && ( | ||
| <div className="shrink-0"> | ||
| {renderCell( | ||
| item[rightColumn.id] as Record<string, unknown>, | ||
| item, | ||
| rightColumn, | ||
| )} | ||
| </div> | ||
| )} | ||
| </div> | ||
| </CardHeader> | ||
| )} | ||
|
|
||
| <CardContent className="flex-1 pb-3"> | ||
| <div className="grid grid-cols-1 md:grid-cols-2 gap-4"> | ||
| <div className="flex flex-col gap-3"> | ||
| {leftColumns.map((column) => { | ||
| const value = item[column.id]; | ||
| const cellContent = renderCell(value as Record<string, unknown>, item, column); | ||
|
|
||
| if (!cellContent) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <div key={String(column.id)} className="flex flex-col gap-1"> | ||
| <Typography variant="small" className="text-muted-foreground"> | ||
| {column.title} | ||
| </Typography> | ||
| <div className="text-sm font-medium">{cellContent}</div> | ||
| </div> | ||
| ); | ||
| })} | ||
| </div> | ||
|
|
||
| <div className="flex flex-col gap-3"> | ||
| {rightColumns.map((column) => { | ||
| const value = item[column.id]; | ||
| const cellContent = renderCell(value as Record<string, unknown>, item, column); | ||
|
|
||
| if (!cellContent) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <div key={String(column.id)} className="flex flex-col gap-1"> | ||
| <Typography variant="small" className="text-muted-foreground"> | ||
| {column.title} | ||
| </Typography> | ||
| <div className="text-sm font-medium">{cellContent}</div> | ||
| </div> | ||
| ); | ||
| })} | ||
| </div> |
There was a problem hiding this comment.
Avoid dropping falsy cell content and recomputing bodyColumns on every row
Two small issues in the body rendering:
if (!cellContent)will also skip valid content such as''or0;renderCellalready returnsnullwhen a cell should be hidden.getBodyColumns()is called inside themap, but its result is independent ofitem, so it can be computed once.
- return (
- <div className={gridClasses}>
- {data.map((item, index) => {
- const rowKey = rowKeyExtractor(item, index);
- const rowClassName = getRowClassName ? getRowClassName(item) : undefined;
- const bodyColumns = getBodyColumns();
+ const bodyColumns = getBodyColumns();
+
+ return (
+ <div className={gridClasses}>
+ {data.map((item, index) => {
+ const rowKey = rowKeyExtractor(item, index);
+ const rowClassName = getRowClassName ? getRowClassName(item) : undefined;
@@
- {leftColumns.map((column) => {
+ {leftColumns.map((column) => {
const value = item[column.id];
const cellContent = renderCell(value as Record<string, unknown>, item, column);
- if (!cellContent) {
+ if (cellContent === null || cellContent === undefined) {
return null;
}
@@
- {rightColumns.map((column) => {
+ {rightColumns.map((column) => {
const value = item[column.id];
const cellContent = renderCell(value as Record<string, unknown>, item, column);
- if (!cellContent) {
+ if (cellContent === null || cellContent === undefined) {
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <div className={gridClasses}> | |
| {data.map((item, index) => { | |
| const rowKey = rowKeyExtractor(item, index); | |
| const rowClassName = getRowClassName ? getRowClassName(item) : undefined; | |
| const bodyColumns = getBodyColumns(); | |
| // Split body columns into two groups for two-column layout | |
| const midPoint = Math.ceil(bodyColumns.length / 2); | |
| const leftColumns = bodyColumns.slice(0, midPoint); | |
| const rightColumns = bodyColumns.slice(midPoint); | |
| return ( | |
| <Card key={rowKey} className={cn('flex flex-col', rowClassName)}> | |
| {(topColumn || leftColumn || rightColumn || bottomColumn) && ( | |
| <CardHeader className="pb-3"> | |
| {topColumn && ( | |
| <div className="mb-2"> | |
| {renderCell(item[topColumn.id] as Record<string, unknown>, item, topColumn)} | |
| </div> | |
| )} | |
| <div className="flex items-start justify-between gap-4"> | |
| {(leftColumn || bottomColumn) && ( | |
| <div className="flex-1 flex flex-col gap-2"> | |
| {leftColumn && ( | |
| <Typography variant="h3" className="font-bold"> | |
| {renderCell( | |
| item[leftColumn.id] as Record<string, unknown>, | |
| item, | |
| leftColumn, | |
| )} | |
| </Typography> | |
| )} | |
| {bottomColumn && ( | |
| <Typography variant="small" className="text-muted-foreground"> | |
| {renderCell( | |
| item[bottomColumn.id] as Record<string, unknown>, | |
| item, | |
| bottomColumn, | |
| )} | |
| </Typography> | |
| )} | |
| </div> | |
| )} | |
| {rightColumn && ( | |
| <div className="shrink-0"> | |
| {renderCell( | |
| item[rightColumn.id] as Record<string, unknown>, | |
| item, | |
| rightColumn, | |
| )} | |
| </div> | |
| )} | |
| </div> | |
| </CardHeader> | |
| )} | |
| <CardContent className="flex-1 pb-3"> | |
| <div className="grid grid-cols-1 md:grid-cols-2 gap-4"> | |
| <div className="flex flex-col gap-3"> | |
| {leftColumns.map((column) => { | |
| const value = item[column.id]; | |
| const cellContent = renderCell(value as Record<string, unknown>, item, column); | |
| if (!cellContent) { | |
| return null; | |
| } | |
| return ( | |
| <div key={String(column.id)} className="flex flex-col gap-1"> | |
| <Typography variant="small" className="text-muted-foreground"> | |
| {column.title} | |
| </Typography> | |
| <div className="text-sm font-medium">{cellContent}</div> | |
| </div> | |
| ); | |
| })} | |
| </div> | |
| <div className="flex flex-col gap-3"> | |
| {rightColumns.map((column) => { | |
| const value = item[column.id]; | |
| const cellContent = renderCell(value as Record<string, unknown>, item, column); | |
| if (!cellContent) { | |
| return null; | |
| } | |
| return ( | |
| <div key={String(column.id)} className="flex flex-col gap-1"> | |
| <Typography variant="small" className="text-muted-foreground"> | |
| {column.title} | |
| </Typography> | |
| <div className="text-sm font-medium">{cellContent}</div> | |
| </div> | |
| ); | |
| })} | |
| </div> | |
| const bodyColumns = getBodyColumns(); | |
| return ( | |
| <div className={gridClasses}> | |
| {data.map((item, index) => { | |
| const rowKey = rowKeyExtractor(item, index); | |
| const rowClassName = getRowClassName ? getRowClassName(item) : undefined; | |
| // Split body columns into two groups for two-column layout | |
| const midPoint = Math.ceil(bodyColumns.length / 2); | |
| const leftColumns = bodyColumns.slice(0, midPoint); | |
| const rightColumns = bodyColumns.slice(midPoint); | |
| return ( | |
| <Card key={rowKey} className={cn('flex flex-col', rowClassName)}> | |
| {(topColumn || leftColumn || rightColumn || bottomColumn) && ( | |
| <CardHeader className="pb-3"> | |
| {topColumn && ( | |
| <div className="mb-2"> | |
| {renderCell(item[topColumn.id] as Record<string, unknown>, item, topColumn)} | |
| </div> | |
| )} | |
| <div className="flex items-start justify-between gap-4"> | |
| {(leftColumn || bottomColumn) && ( | |
| <div className="flex-1 flex flex-col gap-2"> | |
| {leftColumn && ( | |
| <Typography variant="h3" className="font-bold"> | |
| {renderCell( | |
| item[leftColumn.id] as Record<string, unknown>, | |
| item, | |
| leftColumn, | |
| )} | |
| </Typography> | |
| )} | |
| {bottomColumn && ( | |
| <Typography variant="small" className="text-muted-foreground"> | |
| {renderCell( | |
| item[bottomColumn.id] as Record<string, unknown>, | |
| item, | |
| bottomColumn, | |
| )} | |
| </Typography> | |
| )} | |
| </div> | |
| )} | |
| {rightColumn && ( | |
| <div className="shrink-0"> | |
| {renderCell( | |
| item[rightColumn.id] as Record<string, unknown>, | |
| item, | |
| rightColumn, | |
| )} | |
| </div> | |
| )} | |
| </div> | |
| </CardHeader> | |
| )} | |
| <CardContent className="flex-1 pb-3"> | |
| <div className="grid grid-cols-1 md:grid-cols-2 gap-4"> | |
| <div className="flex flex-col gap-3"> | |
| {leftColumns.map((column) => { | |
| const value = item[column.id]; | |
| const cellContent = renderCell(value as Record<string, unknown>, item, column); | |
| if (cellContent === null || cellContent === undefined) { | |
| return null; | |
| } | |
| return ( | |
| <div key={String(column.id)} className="flex flex-col gap-1"> | |
| <Typography variant="small" className="text-muted-foreground"> | |
| {column.title} | |
| </Typography> | |
| <div className="text-sm font-medium">{cellContent}</div> | |
| </div> | |
| ); | |
| })} | |
| </div> | |
| <div className="flex flex-col gap-3"> | |
| {rightColumns.map((column) => { | |
| const value = item[column.id]; | |
| const cellContent = renderCell(value as Record<string, unknown>, item, column); | |
| if (cellContent === null || cellContent === undefined) { | |
| return null; | |
| } | |
| return ( | |
| <div key={String(column.id)} className="flex flex-col gap-1"> | |
| <Typography variant="small" className="text-muted-foreground"> | |
| {column.title} | |
| </Typography> | |
| <div className="text-sm font-medium">{cellContent}</div> | |
| </div> | |
| ); | |
| })} | |
| </div> |
🤖 Prompt for AI Agents
In packages/ui/src/components/DataGrid/DataGrid.tsx around lines 82 to 179,
avoid recomputing body columns per row and avoid dropping falsy but valid cell
content: move const bodyColumns = getBodyColumns() out of the data.map (compute
once before returning JSX) and replace the guard that currently uses if
(!cellContent) with a strict null check (e.g., if (cellContent === null) or
cellContent == null) so empty strings, 0, and false are rendered while only null
hides the cell.
| @@ -1,5 +1,5 @@ | |||
| import { Field, FieldProps, FormikValues } from 'formik'; | |||
| import { Search } from 'lucide-react'; | |||
| import { LayoutGrid, List, Search } from 'lucide-react'; | |||
There was a problem hiding this comment.
Guard item.onChange to avoid potential runtime error
The FilterViewModeToggle branch assumes item.onChange is always provided and uses a non‑null assertion:
onValueChange={(value) => {
if (value === 'list' || value === 'grid') {
item.onChange!(value);
}
}}If a FilterViewModeToggle ever gets created without onChange, this will throw at runtime. You can make this safer without changing behavior:
- onValueChange={(value) => {
- if (value === 'list' || value === 'grid') {
- item.onChange!(value);
- }
- }}
+ onValueChange={(value) => {
+ if ((value === 'list' || value === 'grid') && item.onChange) {
+ item.onChange(value);
+ }
+ }}Also applies to: 203-222
🤖 Prompt for AI Agents
In packages/ui/src/components/Filters/FilterItem.tsx around lines 2 and again
around lines 203-222, the FilterViewModeToggle branch calls item.onChange with a
non-null assertion which can throw if onChange is undefined; update the code to
guard the call by checking that typeof item.onChange === 'function' (or using
optional chaining) before invoking it, or provide a safe no-op fallback so the
component does nothing when onChange is not supplied, preserving current
behavior without risking a runtime error.
| case 'link': { | ||
| const linkDisplayField = column.displayField || 'label'; | ||
| const linkLabel = | ||
| typeof value === 'object' && value !== null ? String(value[linkDisplayField]) : String(value); | ||
| const _href = column.config?.hrefKey ? String(item[column.config.hrefKey]) : '#'; | ||
| const linkVariant = column.config?.variant ? column.config.variant : 'link'; | ||
| const linkClassName = column.config?.className | ||
| ? column.config.className | ||
| : 'flex items-center justify-end gap-2'; | ||
|
|
||
| if (linkVariant === 'link') { | ||
| return ( | ||
| <Link asChild> | ||
| <Button variant="link" className={linkClassName}> | ||
| {linkLabel} | ||
| </Button> | ||
| </Link> | ||
| ); | ||
| } | ||
| return ( | ||
| <Button variant={linkVariant} className={linkClassName}> | ||
| {linkLabel} | ||
| </Button> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Link cells compute _href but never pass it to <Link>, so navigation is broken
In the 'link' case you derive _href but don’t pass it into the <Link> component, so rendered “links” won’t actually navigate anywhere.
- case 'link': {
+ case 'link': {
const linkDisplayField = column.displayField || 'label';
const linkLabel =
typeof value === 'object' && value !== null ? String(value[linkDisplayField]) : String(value);
- const _href = column.config?.hrefKey ? String(item[column.config.hrefKey]) : '#';
+ const href = column.config?.hrefKey ? String(item[column.config.hrefKey]) : '#';
@@
- if (linkVariant === 'link') {
+ if (linkVariant === 'link') {
return (
- <Link asChild>
+ <Link href={href} asChild>
<Button variant="link" className={linkClassName}>
{linkLabel}
</Button>
</Link>
);
}
return (
<Button variant={linkVariant} className={linkClassName}>
{linkLabel}
</Button>
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 'link': { | |
| const linkDisplayField = column.displayField || 'label'; | |
| const linkLabel = | |
| typeof value === 'object' && value !== null ? String(value[linkDisplayField]) : String(value); | |
| const _href = column.config?.hrefKey ? String(item[column.config.hrefKey]) : '#'; | |
| const linkVariant = column.config?.variant ? column.config.variant : 'link'; | |
| const linkClassName = column.config?.className | |
| ? column.config.className | |
| : 'flex items-center justify-end gap-2'; | |
| if (linkVariant === 'link') { | |
| return ( | |
| <Link asChild> | |
| <Button variant="link" className={linkClassName}> | |
| {linkLabel} | |
| </Button> | |
| </Link> | |
| ); | |
| } | |
| return ( | |
| <Button variant={linkVariant} className={linkClassName}> | |
| {linkLabel} | |
| </Button> | |
| ); | |
| } | |
| case 'link': { | |
| const linkDisplayField = column.displayField || 'label'; | |
| const linkLabel = | |
| typeof value === 'object' && value !== null ? String(value[linkDisplayField]) : String(value); | |
| const href = column.config?.hrefKey ? String(item[column.config.hrefKey]) : '#'; | |
| const linkVariant = column.config?.variant ? column.config.variant : 'link'; | |
| const linkClassName = column.config?.className | |
| ? column.config.className | |
| : 'flex items-center justify-end gap-2'; | |
| if (linkVariant === 'link') { | |
| return ( | |
| <Link href={href} asChild> | |
| <Button variant="link" className={linkClassName}> | |
| {linkLabel} | |
| </Button> | |
| </Link> | |
| ); | |
| } | |
| return ( | |
| <Button variant={linkVariant} className={linkClassName}> | |
| {linkLabel} | |
| </Button> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/ui/src/lib/renderCell.tsx around lines 71 to 95, the link case
computes _href but never passes it to the <Link> component so the rendered
button does not navigate; update the <Link> invocation to include the computed
_href (e.g. pass href={_href}) so the wrapped Button becomes a real navigable
link, and consider preserving existing asChild behavior and any target/rel
settings for external URLs if needed.
What does this PR do?
Related Ticket(s)
Key Changes
DataGridcomponent as a tile/card-based alternative view toDataListfor displaying data in a responsive grid layoutpackages/ui/src/components/DataGrid/with full TypeScript types, Storybook stories, and responsive column configuration (mobile/tablet/desktop)DataListfor consistency, with slot-based layout (top, left, right, bottom) for flexible card content arrangementDataListcomponent to extract cell rendering logic into sharedrenderCellutility functionDataList.tsxtopackages/ui/src/lib/renderCell.tsxto enable code reuse betweenDataListandDataGridFilterViewModeTogglefilter type to enable users to switch between list and grid viewspackages/framework/src/utils/models/filters.tswith newFilterViewModeToggleclassFilterscomponent to detect and render view mode toggle alongside other filter itemsFilterItemcomponent to handle view mode toggle rendering with appropriate UI controlsTicketListblock to demonstrate the new featureTicketList.client.tsxwith initial value extraction from filtersDataListorDataGridbased on selected view modeFilterViewModeToggleto control view mode state and persist selectionSide Effects
DataListusages remain unchanged and continue to work as before (backward compatible)Summary by CodeRabbit
New Features
Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.