feat(@angular/forms): add ng-submitted class by edwandr · Pull Request #31070 · angular/angular

edwandr · 2019-06-15T14:40:00Z

Add ng-submitted class to form controls when parent form has been submitted

Fixes #30486

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

No class is added to form controls and container when parent form is submitted

Issue Number: 30486

What is the new behavior?

When parent form is submitted (if there is a parent form), form controls get ng-submitted class

Does this PR introduce a breaking change?

Yes
No

Constructor arguments of NgControlStatus have changed : we added one optional injected argument controlContainer

Other information

Would it be better to add submitted property to Form interface (which NgForm and FormGroupDirective implement) ?

This would avoid the imports of these to directives in ng_control_status.ts. I'm afraid that importing them could increase bundle size.

googlebot · 2019-06-15T14:40:04Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

edwandr · 2019-06-15T14:40:55Z

I signed it!

googlebot · 2019-06-15T14:40:58Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

cexbrayat · 2019-06-15T14:50:00Z

Note to the team: this is a first time contributor, contributing to Angular during the HackCommitPush conference, which aims to help developers to contribute to open source projects :)

CI failure looks like an unrelated flake

jasonaden · 2019-06-20T18:38:48Z

Presubmit

jasonaden

Overall this looks good. Just a couple notes. Thanks for the submission!

jasonaden · 2019-06-27T22:47:00Z

packages/forms/src/directives/ng_control_status.ts

Initially looking at this I was confused because you're checking for this._controlContainer to exist when the constructor here say's required. But in NgControlStatus below you've marked it as @Optional. However, as far as TypeScript goes it's still required.

I would like to make the types align correctly. You will need to use @Inject along with making the ControlContainer optional in order for the JIT compiler to behave. Something like this:

export class AbstractControlStatus { ... constructor(cd: AbstractControlDirective, controlContainer?: ControlContainer) { ... } }

and

export class NgControlStatus extends AbstractControlStatus { constructor(@Self() cd: NgControl, @Inject(ControlContainer) @Optional() controlContainer?: ControlContainer) { ... } }

jasonaden · 2019-06-27T22:58:03Z

tools/public_api_guard/forms/forms.d.ts

Make sure to update the public API docs as well to mark this optional.

mhevery · 2019-07-22T16:56:01Z

@edwandr can you clean up.

edwandr

@jasonaden Thanks for the feedbacks !
@mhevery I'll clean up in the week

edwandr

@jasonaden i added the optional type for controlContainer argument

Add ng-submitted class to form controls when parent form has been submitted Fixes angular#30486

Refs angular#30486

edwandr · 2020-01-06T13:17:40Z

@mhevery @jasonaden Hi, happy new year ! Are the modifications i pushed ok for you ?

AndrewKushnir · 2021-05-06T16:26:27Z

(just for additional context: related issue #30486)

AndrewKushnir

Hi @edwandr, thanks for submitting this PR and sorry that we didn't have a chance to get back to it sooner.

I've reviewed the changes and I think that we should simplify the changes and support the ng-submitted class only for the classes that actually have this status, i.e. NgForm and FormGroupDirective (the directives that can represent the top-level form). Other form elements (like <input>s) would be able to use the top-level form's status to set specific styling if needed (for ex.: .ng-submitted input { font-size: 10px; }).

Please let us know if you might be interested in making additional changes in this PR.

Thank you.

AndrewKushnir · 2021-05-18T22:16:32Z

Closing this PR, new PR - #42132.

angular-automatic-lock-bot · 2021-06-18T16:06:29Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

edwandr requested review from a team June 15, 2019 14:40

googlebot added the cla: no label Jun 15, 2019

googlebot added cla: yes and removed cla: no labels Jun 15, 2019

cexbrayat changed the title ~~Feat/add ng submitted~~ feat(@angular/forms): add ng-submitted class Jun 15, 2019

kara added the area: forms label Jun 19, 2019

ngbot bot added this to the needsTriage milestone Jun 19, 2019

jasonaden added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Jun 27, 2019

jasonaden suggested changes Jun 27, 2019

View reviewed changes

kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 17, 2019

edwandr commented Jul 29, 2019

View reviewed changes

jorroll mentioned this pull request Aug 2, 2019

A proposal to improve ReactiveFormsModule #31963

Open

kara removed the action: merge The PR is ready for merge by the caretaker label Aug 9, 2019

edwandr force-pushed the feat/add-ng-submitted branch from 08fe213 to 67728a7 Compare August 9, 2019 19:51

edwandr commented Aug 9, 2019

View reviewed changes

edwandr force-pushed the feat/add-ng-submitted branch from 67728a7 to 0200127 Compare August 9, 2019 20:57

edwandr force-pushed the feat/add-ng-submitted branch from 0200127 to 40ff640 Compare August 27, 2019 18:54

edwandr force-pushed the feat/add-ng-submitted branch from 40ff640 to f538908 Compare November 7, 2019 16:34

edwandr added 2 commits November 7, 2019 17:44

feat(forms): add ng-submitted class

f08e784

Add ng-submitted class to form controls when parent form has been submitted Fixes angular#30486

docs(forms): add ng-submitted class to form documentation

3c7aca1

Refs angular#30486

edwandr force-pushed the feat/add-ng-submitted branch from f538908 to 3c7aca1 Compare November 7, 2019 16:44

pullapprove bot requested a review from AndrewKushnir May 6, 2021 18:16

AndrewKushnir requested changes May 7, 2021

View reviewed changes

dylhunn mentioned this pull request May 17, 2021

feat(forms): add ng-submitted class to forms that have been submitted. #42132

Closed

14 tasks

AndrewKushnir closed this May 18, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 18, 2021

Conversation

edwandr commented Jun 15, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

googlebot commented Jun 15, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

edwandr commented Jun 15, 2019

Uh oh!

googlebot commented Jun 15, 2019

Uh oh!

cexbrayat commented Jun 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonaden commented Jun 20, 2019

Uh oh!

jasonaden left a comment

Choose a reason for hiding this comment

Uh oh!

jasonaden Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

jasonaden Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

mhevery commented Jul 22, 2019

Uh oh!

edwandr left a comment

Choose a reason for hiding this comment

Uh oh!

edwandr left a comment

Choose a reason for hiding this comment

Uh oh!

edwandr commented Jan 6, 2020

Uh oh!

AndrewKushnir commented May 6, 2021

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented May 18, 2021

Uh oh!

angular-automatic-lock-bot bot commented Jun 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cexbrayat commented Jun 15, 2019 •

edited

Loading