fix(module:input): update disable logic to support Angular Signal Forms#9701
fix(module:input): update disable logic to support Angular Signal Forms#9701Nicoss54 wants to merge 1 commit intoNG-ZORRO:masterfrom
Conversation
Summary of ChangesHello @Nicoss54, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a compatibility issue within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request addresses a bug related to the disabled logic in NzInputDirective to support Angular Signal Forms. The change introduces linkedSignal to reactively update the controlDisabled state, which is a good step towards better integration with Angular's signal-based reactivity. The comments in the code explain the rationale behind this change, acknowledging it as a temporary workaround until Signal Forms API stabilizes. The changes are focused and directly address the described bug.
components/input/input.directive.ts
Outdated
| // Angular Signal Forms uses `InteropNgControl` which does not emit `statusChanges`. | ||
| // However, `InteropNgControl.disabled` is signal-reactive (it reads from the field's | ||
| // computed disabled signal), so an `effect()` here picks up changes for Signal Forms | ||
| // while remaining a one-shot read for traditional reactive forms (whose updates | ||
| // continue to come from the `statusChanges` subscription in `ngOnInit`). | ||
| // I think this is "nice" workaround for now, but we should find a better solution in the future when signal form API will be stable. | ||
| readonly controlDisabled = linkedSignal(() => !!this.ngControl?.disabled || false); |
There was a problem hiding this comment.
The extensive comment block here is helpful for understanding the immediate problem and the chosen workaround. However, it's quite verbose for an inline code comment. Consider shortening it or moving some of the detailed explanation to a separate documentation file or a more high-level comment if this is a known temporary solution. While the explanation is good, the code should ideally be self-documenting or have concise comments.
components/input/input.directive.ts
Outdated
| // while remaining a one-shot read for traditional reactive forms (whose updates | ||
| // continue to come from the `statusChanges` subscription in `ngOnInit`). | ||
| // I think this is "nice" workaround for now, but we should find a better solution in the future when signal form API will be stable. | ||
| readonly controlDisabled = linkedSignal(() => !!this.ngControl?.disabled || false); |
There was a problem hiding this comment.
The use of linkedSignal is appropriate here for creating a signal that derives its value from another signal (this.ngControl?.disabled). This aligns with the Angular best practices for state management using signals, as outlined in the repository's style guide (lines 32-33). The !! operator ensures a boolean value, which is good for consistency.
References
- Use signals for state management and computed() for derived state. (link)
There was a problem hiding this comment.
yeah it seems there is a miss in Angula documentation but in the case of signal form this.ngControl?.disabled is a signal
There was a problem hiding this comment.
Pull request overview
This PR fixes the disabled state handling in the input directive to support Angular Signal Forms. The change updates controlDisabled from a simple signal to a linkedSignal that reactively tracks ngControl?.disabled, which works with both traditional reactive forms (via the existing statusChanges subscription) and newer Signal Forms (via reactive signal tracking).
Changes:
- Modified
controlDisabledto uselinkedSignal()with a computation function that readsngControl?.disabled - Added detailed comment explaining the workaround for Signal Forms compatibility
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9701 +/- ##
=======================================
Coverage 90.10% 90.10%
=======================================
Files 574 574
Lines 23671 23671
Branches 4814 4814
=======================================
+ Hits 21328 21329 +1
Misses 1520 1520
+ Partials 823 822 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Nicoss54
force-pushed
the
fix/disable-signal-forms
branch
from
33717eb to
4a9a0aa
Compare
|
@HyperLife1119 @Laffery when you have time could you also take a look ? :) |
|
I'm not sure if it's a good idea to be compatible with signal form before it's stable (at least in developer preview), but I've seen a PR that will implement statusChanges for InteropNgControl. |
+1 |
|
@HyperLife1119 @Laffery I also agree with both of you ! This was just a quick win to unblock the issue @Laffery mentionned. But agree to close it |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information