Skip to content

Conversation

@spike-rabbit
Copy link
Member

@spike-rabbit spike-rabbit commented Nov 17, 2025

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

rows is a decorator input.

What is the new behavior?

rows is a signal input.

Does this PR introduce a breaking change? (check one with "x")

  • Yes, but message is contained in another commit
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications:

Other information:

Turns _internalRows into a signal. There is more potential once groupedRows is a signal as well.

@spike-rabbit spike-rabbit force-pushed the refactor/datatable/make-rows-a-signal branch 3 times, most recently from 8a2caf2 to f0ac168 Compare November 18, 2025 06:48
@spike-rabbit spike-rabbit marked this pull request as ready for review November 18, 2025 06:54
@spike-rabbit spike-rabbit force-pushed the refactor/datatable/make-rows-a-signal branch from f0ac168 to f9dbe46 Compare November 18, 2025 06:54
@spike-rabbit spike-rabbit requested review from a team as code owners November 18, 2025 06:54
@spike-rabbit
Copy link
Member Author

spike-rabbit commented Nov 18, 2025

@fh1ch the sorting now behaves slightly differently. Before:

// The same array will be sorted each time the users sorts.
// Assuming the are values, that have the same sort order.
// Then we have different results based on the order in which a user sorts columns
[...].sort().sort().sort()

After

// The original array is copied.
// We always get the same result.
// Independent of the order in which a user sorts.
const rows = [...];
rows.slice().sort();
rows.slice().sort();
rows.slice().sort();

@fh1ch fh1ch requested a review from Copilot November 18, 2025 14:07
@fh1ch fh1ch added the breaking-changes Marks issues and PRs that are breaking the API label Nov 18, 2025
Copilot finished reviewing on behalf of fh1ch November 18, 2025 14:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the rows input from a decorator-based input with getter/setter to an Angular signal input, and converts _internalRows from a plain array to a computed signal. This is part of a broader migration to Angular's signals-based reactivity system.

Key Changes

  • Converted rows from @Input() decorator to input<>() signal
  • Converted _internalRows from a mutable array to a computed() signal that handles sorting, tree grouping, and ghost loading
  • Removed the _rows backing field and translateColumns() method as they're no longer needed
  • Updated all references to rows and _internalRows to call them as functions

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
projects/ngx-datatable/src/lib/components/datatable.component.ts Refactored rows to signal input, converted _internalRows to computed signal with internal logic for sorting and tree grouping, removed obsolete helper methods and effects
projects/ngx-datatable/src/lib/components/datatable.component.spec.ts Updated test expectations to reflect changes in sort behavior (appears to be regression)
projects/ngx-datatable/src/lib/components/datatable.component.html Updated template bindings to call _internalRows() as a function instead of accessing it as a property

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@spike-rabbit spike-rabbit force-pushed the refactor/datatable/make-rows-a-signal branch from f9dbe46 to 07bac2f Compare November 18, 2025 14:39
Copy link
Member

@fh1ch fh1ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spike-rabbit nice one, thanks a lot 🙇

LGTM 👍

As part of this change, `_internalRows` are now
a computed signal.
Further optimizations are done
after rowGroups are turned into a signal as well.
@fh1ch fh1ch force-pushed the refactor/datatable/make-rows-a-signal branch from 07bac2f to 722eac7 Compare November 19, 2025 07:56
@fh1ch fh1ch enabled auto-merge (rebase) November 19, 2025 07:57
@fh1ch fh1ch merged commit fc36697 into main Nov 19, 2025
3 checks passed
@fh1ch fh1ch deleted the refactor/datatable/make-rows-a-signal branch November 19, 2025 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-changes Marks issues and PRs that are breaking the API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants