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.

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

feat(@angular/forms): add ng-submitted class #31070

Closed
wants to merge 2 commits into from

Conversation

@edwandr
Copy link

@edwandr edwandr commented Jun 15, 2019

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:

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.

@edwandr edwandr requested review from as code owners Jun 15, 2019
@googlebot
Copy link

@googlebot googlebot commented Jun 15, 2019

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
Corporate signers

ℹ️ Googlers: Go here for more info.

Loading

@edwandr
Copy link
Author

@edwandr edwandr commented Jun 15, 2019

I signed it!

Loading

@googlebot
Copy link

@googlebot googlebot commented Jun 15, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Loading

@cexbrayat
Copy link
Contributor

@cexbrayat cexbrayat commented Jun 15, 2019

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

Loading

@cexbrayat cexbrayat changed the title Feat/add ng submitted feat(@angular/forms): add ng-submitted class Jun 15, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jun 19, 2019
@jasonaden
Copy link
Contributor

@jasonaden jasonaden commented Jun 20, 2019

Loading

Copy link
Contributor

@jasonaden jasonaden left a comment

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

Loading

@@ -24,6 +30,11 @@ export class AbstractControlStatus {
get ngClassValid(): boolean { return this._cd.control ? this._cd.control.valid : false; }
get ngClassInvalid(): boolean { return this._cd.control ? this._cd.control.invalid : false; }
get ngClassPending(): boolean { return this._cd.control ? this._cd.control.pending : false; }
get ngClassSubmitted(): boolean {
return this._controlContainer && this._controlContainer.formDirective ?
Copy link
Contributor

@jasonaden jasonaden Jun 27, 2019

Choose a reason for hiding this comment

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

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) { ... }
}

Loading

@@ -360,7 +360,7 @@ export declare abstract class NgControl extends AbstractControlDirective {
}

export declare class NgControlStatus extends AbstractControlStatus {
constructor(cd: NgControl);
constructor(cd: NgControl, controlContainer: ControlContainer);
Copy link
Contributor

@jasonaden jasonaden Jun 27, 2019

Choose a reason for hiding this comment

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

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

Loading

@mhevery
Copy link
Contributor

@mhevery mhevery commented Jul 22, 2019

@edwandr can you clean up.

Loading

Copy link
Author

@edwandr edwandr left a comment

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

Loading

Copy link
Author

@edwandr edwandr left a comment

@jasonaden i added the optional type for controlContainer argument

Loading

@edwandr edwandr force-pushed the feat/add-ng-submitted branch from 67728a7 to 0200127 Aug 9, 2019
@edwandr edwandr force-pushed the feat/add-ng-submitted branch from 0200127 to 40ff640 Aug 27, 2019
@edwandr edwandr force-pushed the feat/add-ng-submitted branch from 40ff640 to f538908 Nov 7, 2019
edwandr added 2 commits Nov 7, 2019
Add ng-submitted class to form controls when parent form has been submitted

Fixes angular#30486
@edwandr edwandr force-pushed the feat/add-ng-submitted branch from f538908 to 3c7aca1 Nov 7, 2019
@edwandr
Copy link
Author

@edwandr edwandr commented Jan 6, 2020

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

Loading

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented May 6, 2021

(just for additional context: related issue #30486)

Loading

@pullapprove pullapprove bot requested a review from AndrewKushnir May 6, 2021
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

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.

Loading

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented May 18, 2021

Closing this PR, new PR - #42132.

Loading

@angular-automatic-lock-bot
Copy link

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

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.

Loading

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants