Fix/463 table design review - #527
Conversation
* controls can be now added to end of table (collapse as last col e.g.)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds table control-column ordering with a ChangesTable Features and Demos
Responsive Pagination
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TediTableComponent
participant BreakpointService
participant ModalService
participant TableFilterModalComponent
User->>TediTableComponent: select column filter
TediTableComponent->>BreakpointService: read current breakpoint
TediTableComponent->>ModalService: open TableFilterModalComponent
ModalService->>TableFilterModalComponent: provide filter data
User->>TableFilterModalComponent: apply or clear filter
TableFilterModalComponent->>TediTableComponent: build filter context
TableFilterModalComponent->>ModalService: close modal
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.14.0)tedi/components/navigation/pagination/pagination.component.scssError: Cannot resolve module "/node_modules/stylelint-scss/src/index.js". Check that module "/node_modules/stylelint-scss/src/index.js" is available and spelled correctly Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tedi/components/content/table/table.stories.ts (1)
714-720: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared
dateRangecolumn factory to avoid repeating the same object 7 times.The
{ id: "dateRange", header: "Kuupäev", accessorKey: "dateRange", minSize: 210, cell: bookingDateRangeCell }block (plus its sortingFn variant at Lines 998-1004) is duplicated across 7 stories. A shared helper co-located withbookingDateRangeCellintable-demo-data.tswould reduce drift risk if the column definition ever needs to change.♻️ Proposed factory (in table-demo-data.ts)
+export function bookingDateRangeColumn( + overrides: Partial<TediColumnDef<Booking>> = {}, +): TediColumnDef<Booking> { + return { + id: "dateRange", + header: "Kuupäev", + accessorKey: "dateRange", + minSize: 210, + cell: bookingDateRangeCell, + ...overrides, + } as TediColumnDef<Booking>; +}Then in
table.stories.ts:- { - id: "dateRange", - header: "Kuupäev", - accessorKey: "dateRange", - minSize: 210, - cell: bookingDateRangeCell, - } as TediColumnDef<Booking>, + bookingDateRangeColumn(),And for the
MergedCellsvariant (Lines 998-1004):- minSize: 210, - sortable: true, - sortingFn: (a, b) => - (a.original.dateRange.from?.getTime() ?? 0) - - (b.original.dateRange.from?.getTime() ?? 0), - cell: bookingDateRangeCell, - } as TediColumnDef<Booking>, + bookingDateRangeColumn({ + sortable: true, + sortingFn: (a, b) => + (a.original.dateRange.from?.getTime() ?? 0) - + (b.original.dateRange.from?.getTime() ?? 0), + }),Also applies to: 798-804, 896-902, 998-1004, 4285-4291, 4358-4364, 4421-4427
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tedi/components/content/table/table.stories.ts` around lines 714 - 720, The dateRange column definition is duplicated across multiple table stories, including the bookingDateRangeCell and sortingFn variant, which creates drift risk. Extract a shared dateRange column factory alongside bookingDateRangeCell in table-demo-data.ts, then replace each repeated TediColumnDef<Booking> object in table.stories.ts with calls to that helper; keep the MergedCells sorting variant supported by the same factory so all 7 usages stay consistent.tedi/components/content/table/table.component.spec.ts (1)
1707-1714: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also asserting the
fullscreenmodal option.The test checks
data.columnLabelbut not thefullscreenfield passed tomodalService.open(...), which is also new behavior (filterModalFullscreen). A quick assertion would close a small coverage gap for this PR's feature surface.✅ Suggested addition
expect(modalOpen).toHaveBeenCalledTimes(1); expect(modalOpen.mock.calls[0][0]).toBe(TableFilterModalComponent); expect(modalOpen.mock.calls[0][1].data.columnLabel).toBe("Name"); + expect(modalOpen.mock.calls[0][1].fullscreen).toBe(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tedi/components/content/table/table.component.spec.ts` around lines 1707 - 1714, The filter modal test in TableComponentSpec only verifies the modal data payload and is missing coverage for the new fullscreen behavior. Update the spec around the modalService.open call in the “opens the filter in a modal below the breakpoint” case to also assert the passed fullscreen option reflects the new filterModalFullscreen behavior, alongside the existing TableFilterModalComponent and columnLabel checks.tedi/components/content/table/table.component.ts (1)
2231-2266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider routing the popover's Apply/Clear buttons through the shared context builder.
buildFilterContext'sapply/clearclosures now duplicate the logic already inhandleFilterApply/handleFilterClear(lines 2305-2321): both dosetFilterValue(draft())+ close, ordraft.set(undefined)+setFilterValue(undefined)+ close. The popover footer buttons intable.component.htmlstill call the olderhandleFilterApply/handleFilterClearmethods directly instead offilterContextFor(...).apply()/.clear(). Two parallel implementations of the same commit/close behavior risk drifting apart on future changes (e.g. adding validation before commit).♻️ Proposed consolidation
- <button - tedi-button - variant="secondary" - size="small" - type="button" - (click)=" - handleFilterClear( - header.column, - filterPopover - ) - " - > + <button + tedi-button + variant="secondary" + size="small" + type="button" + (click)="filterContextFor(header.column, filterPopover).clear()" + > {{ filterClearLabel() }} </button> - <button - tedi-button - variant="primary" - size="small" - type="button" - (click)=" - handleFilterApply( - header.column, - filterPopover - ) - " - > + <button + tedi-button + variant="primary" + size="small" + type="button" + (click)="filterContextFor(header.column, filterPopover).apply()" + > {{ filterApplyLabel() }} </button>Then
handleFilterApply/handleFilterClearcan be removed fromtable.component.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tedi/components/content/table/table.component.ts` around lines 2231 - 2266, The popover Apply/Clear actions are still wired to the legacy `handleFilterApply`/`handleFilterClear` methods instead of the shared `buildFilterContext` flow, which duplicates commit/close behavior. Update the popover footer in `table.component.html` to call `filterContextFor(...).apply()` and `.clear()` through the context returned by `buildFilterContext`, so both popover and modal use the same logic. Once that is wired through `filterContextFor`/`buildFilterContext`, the redundant `handleFilterApply` and `handleFilterClear` methods in `table.component.ts` can be removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tedi/components/content/table/table-demo.constants.ts`:
- Around line 118-125: The tedi-date-field binding in the table demo does not
accept cleared null values, but DateFieldComponent can emit null through its CVA
when the range is cleared. Update the draft update flow around
editor.setDraftValueRaw and editor.draftValueRaw so the dateRange path accepts
T[K] | null, or coerce the emitted value before passing it through, keeping the
tedi-date-field integration compatible with nullable clears.
---
Nitpick comments:
In `@tedi/components/content/table/table.component.spec.ts`:
- Around line 1707-1714: The filter modal test in TableComponentSpec only
verifies the modal data payload and is missing coverage for the new fullscreen
behavior. Update the spec around the modalService.open call in the “opens the
filter in a modal below the breakpoint” case to also assert the passed
fullscreen option reflects the new filterModalFullscreen behavior, alongside the
existing TableFilterModalComponent and columnLabel checks.
In `@tedi/components/content/table/table.component.ts`:
- Around line 2231-2266: The popover Apply/Clear actions are still wired to the
legacy `handleFilterApply`/`handleFilterClear` methods instead of the shared
`buildFilterContext` flow, which duplicates commit/close behavior. Update the
popover footer in `table.component.html` to call `filterContextFor(...).apply()`
and `.clear()` through the context returned by `buildFilterContext`, so both
popover and modal use the same logic. Once that is wired through
`filterContextFor`/`buildFilterContext`, the redundant `handleFilterApply` and
`handleFilterClear` methods in `table.component.ts` can be removed.
In `@tedi/components/content/table/table.stories.ts`:
- Around line 714-720: The dateRange column definition is duplicated across
multiple table stories, including the bookingDateRangeCell and sortingFn
variant, which creates drift risk. Extract a shared dateRange column factory
alongside bookingDateRangeCell in table-demo-data.ts, then replace each repeated
TediColumnDef<Booking> object in table.stories.ts with calls to that helper;
keep the MergedCells sorting variant supported by the same factory so all 7
usages stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac4c6b92-5403-47e6-99f6-de6af30a9d99
📒 Files selected for processing (12)
skills/tedi-angular/references/components.mdtedi/components/content/table/table-demo-data.tstedi/components/content/table/table-demo.constants.tstedi/components/content/table/table-filter-modal/table-filter-modal.component.htmltedi/components/content/table/table-filter-modal/table-filter-modal.component.tstedi/components/content/table/table.component.htmltedi/components/content/table/table.component.scsstedi/components/content/table/table.component.spec.tstedi/components/content/table/table.component.tstedi/components/content/table/table.stories.tstedi/components/content/table/table.types.tstedi/components/navigation/pagination/pagination.component.scss
| <tedi-date-field | ||
| [inputId]="'dateRange-' + ctx.row.original.id" | ||
| size="small" | ||
| mode="range" | ||
| placeholder="pp.kk.aaaa – pp.kk.aaaa" | ||
| [ngModel]="editor.draftValueRaw(ctx.row.original.id, 'dateRange')" | ||
| (ngModelChange)="editor.setDraftValueRaw(ctx.row.original.id, 'dateRange', $event)" | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect DateFieldComponent's value/CVA type to confirm ngModelChange emission type.
fd -e ts date-field.component --exec cat -n {}Repository: TEDI-Design-System/angular
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the table demo constants and the date field component/type definitions.
git ls-files 'tedi/components/content/table/table-demo.constants.ts' \
'tedi/components/**/*.ts' | rg 'date-field\.component|calendar/types|table-demo\.constants|date-field'
echo '--- table-demo.constants.ts (relevant region) ---'
sed -n '200,260p' tedi/components/content/table/table-demo.constants.ts
echo '--- date-field.component.ts ---'
sed -n '1,260p' tedi/components/form/date-field/date-field.component.ts
echo '--- date type definitions ---'
sed -n '1,220p' tedi/components/content/calendar/types.tsRepository: TEDI-Design-System/angular
Length of output: 13072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the editable table story's row type and the date-range column binding.
rg -n -C 4 'EditableValues|dateRange|setDraftValueRaw|draftValueRaw|tedi-date-field' \
tedi/components/content/table/table.stories.ts \
tedi/components/content/table/table-demo.constants.tsRepository: TEDI-Design-System/angular
Length of output: 21158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the Booking row type and whether dateRange is nullable.
rg -n -C 3 'type Booking|interface Booking|dateRange:' tedi/components/content/table/table.stories.tsRepository: TEDI-Design-System/angular
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the Booking type and its dateRange field.
rg -n -C 4 'interface Booking|type Booking|dateRange' tedi/components/content/tableRepository: TEDI-Design-System/angular
Length of output: 16338
Allow nullable clears from tedi-date-field here. DateFieldComponent’s CVA value includes null, so clearing a range can emit null, but setDraftValueRaw(..., value: T[K]) only accepts DateRange. Widen the setter to T[K] | null or coerce before forwarding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tedi/components/content/table/table-demo.constants.ts` around lines 118 -
125, The tedi-date-field binding in the table demo does not accept cleared null
values, but DateFieldComponent can emit null through its CVA when the range is
cleared. Update the draft update flow around editor.setDraftValueRaw and
editor.draftValueRaw so the dateRange path accepts T[K] | null, or coerce the
emitted value before passing it through, keeping the tedi-date-field integration
compatible with nullable clears.
* added align input * flexbox instead of grid
GitHub Actions
Inactive
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation