Skip to content

Conversation

@spike-rabbit
Copy link
Member

@spike-rabbit spike-rabbit commented Nov 3, 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)

Some @Input with default assignment in the constructor are not signals yet.

What is the new behavior?

All @Input from the constructor are now signal inputs and are defaulted at initialization. The constructor is removed and related code places are adjusted.

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

  • Yes
  • No

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

For breaking change note refer to 4bfbf62.

Other information:

@spike-rabbit spike-rabbit requested a review from a team as a code owner November 3, 2025 10:21
@fh1ch fh1ch added the breaking-changes Marks issues and PRs that are breaking the API label Nov 3, 2025
@fh1ch fh1ch requested a review from Copilot November 3, 2025 11:02
@spike-rabbit spike-rabbit force-pushed the refactor/datatable/convert-constructor-assigned-inputs-to-signals branch from 4fc8414 to fe2c5b5 Compare November 3, 2025 11:04
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 migrates several properties from Angular @Input() decorators to Angular signal-based input() functions in the DatatableComponent. The migration affects rowHeight, headerHeight, footerHeight, cssClasses, and messages properties, converting them from traditional inputs to signal inputs. The constructor logic that initialized these properties from global configuration has been removed and integrated directly into the input definitions.

Key changes:

  • Converted five @Input() properties to signal-based input() functions with configuration defaults
  • Updated all template usages and component code to call these as functions (e.g., headerHeight() instead of headerHeight)
  • Removed the constructor that was previously initializing these properties from configuration
  • Updated tests to accommodate the signal-based API, including proper mocking of the messages signal

Reviewed Changes

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

Show a summary per file
File Description
datatable.component.ts Migrated five properties from @Input() to input() signals and removed constructor initialization logic
datatable.component.html Updated all references to signal properties to use function call syntax
pager.component.ts Changed messages getter to properly invoke the signal function and updated type annotation
pager.component.spec.ts Updated test setup to mock messages as a signal and modified test helper to properly set signal values
footer.component.spec.ts Added messages signal to test fixture component to maintain compatibility

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

@spike-rabbit spike-rabbit force-pushed the refactor/datatable/convert-constructor-assigned-inputs-to-signals branch from fe2c5b5 to 9c61d5a Compare November 3, 2025 11:13
@fh1ch fh1ch requested a review from Copilot November 3, 2025 18:33
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

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


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

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