Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(material/chips): chips form control updating value immediately #30694

Closed
wants to merge 98 commits into from
Closed

fix(material/chips): chips form control updating value immediately #30694

wants to merge 98 commits into from
+21,604 −29,232

Conversation

mistrykaran91
Copy link
Contributor

Currently, when we have chips with form control, the value is updated only when its focused out. This fix will update the value of form control immediately

Fixes #28065

@mistrykaran91 mistrykaran91 requested a review from a team as a code owner March 24, 2025 08:21
@mistrykaran91 mistrykaran91 requested review from mmalerba and andrewseguin and removed request for a team March 24, 2025 08:21
@mistrykaran91 mistrykaran91 force-pushed the fix/chips-form-control-value-update branch 5 times, most recently from 8393040 to 3496154 Compare March 24, 2025 09:04
mmalerba
mmalerba requested changes Mar 27, 2025
View reviewed changes
@mistrykaran91
fix(material/chips): chips form control updating value immediately
eb7ee2b 
Currently, when we have chips with form control, the value is updated only when its focused out. This fix will update the value of form control immediately

Fixes angular#28065
@mistrykaran91 mistrykaran91 force-pushed the fix/chips-form-control-value-update branch from 3496154 to eb7ee2b Compare March 28, 2025 03:10
@mistrykaran91 mistrykaran91 requested a review from mmalerba March 28, 2025 03:28
@mistrykaran91
Copy link
Contributor Author

@mmalerba @crisbeto : Just out of the context question, aren't we looking to migrate @Input toinput query signals ? If yes, then can I create PR for it component by component at a time ?

@mmalerba
Copy link
Contributor

Just out of the context question, aren't we looking to migrate @Input to input query signals ? If yes, then can I create PR for it component by component at a time ?

My only worry with that is that people are probably programmatically accessing the @Input property, so changing it to a signal would be a breaking change

@mistrykaran91
Copy link
Contributor Author

Just out of the context question, aren't we looking to migrate @Input to input query signals ? If yes, then can I create PR for it component by component at a time ?

My only worry with that is that people are probably programmatically accessing the @Input property, so changing it to a signal would be a breaking change

So, as of now we won't be doing any input signal migration now. But if there is some decision on that to migrate one by one with breaking change note I can contribute a bit for smaller component atleast.

@mmalerba
Copy link
Contributor

@mistrykaran91 sure, thanks for volunteering! I'll definitely let you know if there's a chance to help with that

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Apr 1, 2025
@mmalerba mmalerba self-assigned this Apr 1, 2025
@mmalerba
Copy link
Contributor

mmalerba commented Apr 2, 2025

FYI this breaks a handful of tests in google, We'll need to investigate and determine if the issue is just with brittle tests or something with the change itself.

@mmalerba mmalerba added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 2, 2025
@mistrykaran91
Copy link
Contributor Author

FYI this breaks a handful of tests in google, We'll need to investigate and determine if the issue is just with brittle tests or something with the change itself.

Let me know if this change does not work, I at-least think this is the place but you can let me know if its because of the change I can revisit.

devversion and others added 6 commits April 7, 2025 10:00
@devversion @mistrykaran91
build: update angular-linking to attempt fix postinstall flakiness (a…
21cf14a 
…ngular#30698)

This version of `angular-linking` has the new license (MIT), and also
disables Babel config lookups. That potentially fixes the flakiness
where Babel fails reading some incomplete config file
@crisbeto @mistrykaran91
fix(material/sort): set cursor on entire clickable area (angular#30696)
7098e66 
Sets the `cursor` on the entire sort header, rather than the inner container, since the whole element is clickable.

Fixes angular#30690.
@devversion @mistrykaran91
build: migrate theming tests to rules_js
d786b43 
Migrates the theming tests to `rules_js`. This also requires us to bring
in the Jasmine ruleset, and fix some issues with the test tsconfig.
@devversion @mistrykaran91
build: migrate more targets to ts_project
04badba 
This commit migrates more targets from `ts_library` to `ts_project`.
@devversion @mistrykaran91
build: include src/cdk in pnpm workspace
052ebc1 
This will allow pnpm to identifiy dependencies like `parse5` to be
understood as non-dev dependencies, so that they are propagated with
`rules_js` even when `cdk` is linked as `npm_package`. This is necessary
when using the npm package of CDK in the Material schematic tests.

To do this we need to give up on our non-statically readable
substitutions for tslib and RxJS but this is acceptable as those don't
change often and the benefits of the pnpm workspace are more important.

We move `parse5` from `optionalDependencies` to `dependencies` so that
pnpm recognizes this as non-dev dependency for the runtime execution of
the CDK package. `parse5` doesn't have any native code, so
`optionalDependencies` vs `dependencies` doesn't make a difference.

Long-term we should consider simply bundling `parse5` here.
@devversion @mistrykaran91
build: migrate schematics to ts_project
1d72be8 
Migrates all schematics code to `ts_project` and simplifies
complexity/confusion around devmode,prodmode ESM and CommonJS.
andrewseguin and others added 13 commits April 7, 2025 10:02
@andrewseguin @mistrykaran91
refactor: remove extraneous log (angular#30799)
6f687dc
@crisbeto @mistrykaran91
feat(material/core): handle prefers-reduced-motion automatically (ang…
99a9365 
…ular#30796)

Adds some logic so that animations automatically get disabled if the `prefers-reduced-motion` media query matches.
@angular-robot @mistrykaran91
build: update angular shared dev-infra code to 492cc1f (angular#30798)
06377fe
@angular-robot @mistrykaran91
build: update angular shared dev-infra code to f172f3f (angular#30801)
75a31c3
@wagnermaciel @mistrykaran91
fix(cdk-experimental/ui-patterns): add missing event handlers (angula…
05f4503 
…r#30786)

* Add missing event handlers for alternative multiselection strategy
@mistrykaran91
fix(material-angular-io): streamline directive and component metadata…
e2b64d6 
… handling in dgeni (angular#30661)

Currently, docs is not grouped based on Components and Directives. This fix will separate those two sections.

Fixes angular#24093
@andrewseguin @mistrykaran91
fix(multiple): rename token prefixes to match components (angular#30802)
6d64113 
* fix(multiple): rename token prefixes to match components

* fix(material/core): theming tests
@wagnermaciel @mistrykaran91
refactor(cdk-experimental/listbox): rename multiselectable to multi (a…
eee2e47 
…ngular#30804)
@angular-robot @mistrykaran91
build: update angular shared dev-infra code to e4bf37a (angular#30805)
dbafdd0
@wagnermaciel @mistrykaran91
feat(cdk-experimental/listbox): readonly mode (angular#30803)
7536963 
* feat(cdk-experimental/listbox): readonly mode

* fixup! feat(cdk-experimental/listbox): readonly mode
@dominicbachmann @mistrykaran91
docs(material/chips): fix aria-label binding of chips-form-control-ex…
3419e97 
…ample (angular#30811)

Fixes the aria-label binding of the remove keyword button in chips-form-control-example html. The aria-label used string concatenation with an attribute instead of a property binding.
@andrewseguin @mistrykaran91
fix(multiple): rename hardcoded tokens that were renamed (angular#30807)
334e60d 
* fix(multiple): rename hardcoded tokens that were renamed

* fix(multiple): fix density prefixes

* fix(multiple): fix density prefixes

---------

Co-authored-by: Andrew Seguin <[email protected]>
@mistrykaran91
docs(material/chips): fix aria label binding for reactive and templat…
53d114c 
…e driven form (angular#30814)

Fixes the aria-label binding of the remove keyword button in chips reative and template driven form. The aria-label used string concatenation with an attribute instead of a property binding.
@mistrykaran91 mistrykaran91 requested a review from a team as a code owner April 7, 2025 04:32
@mistrykaran91
Copy link
Contributor Author

Closing this MR, while rebasing seems to be getting corrupted, will raise a new one

@mistrykaran91 mistrykaran91 deleted the fix/chips-form-control-value-update branch April 7, 2025 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmalerba mmalerba mmalerba approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin andrewseguin is a code owner automatically assigned from angular/components-googlers

Assignees

@mmalerba mmalerba

Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project area: cdk/drag-drop area: cdk/overlay area: docs Related to the documentation area: material/checkbox area: material/chips area: material/core area: material/form-field area: material/slider area: material/snack-bar area: material/sort area: material/tabs area: performance Issues related to performance detected: feature PR contains a feature commit presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug(MatChipGrid): Chips form controls do not update values until focus is achieved and lost