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

feat(forms): add option to use native validation and angular forms #13566

Merged
merged 1 commit into from
Feb 21, 2017
Merged

feat(forms): add option to use native validation and angular forms #13566

merged 1 commit into from
Feb 21, 2017

Conversation

DzmitryShylovich
Copy link
Contributor

@DzmitryShylovich DzmitryShylovich commented Dec 19, 2016 •
edited

Closes #13573

@DzmitryShylovich DzmitryShylovich force-pushed the gh/no-novalidate branch 3 times, most recently from 25209f0 to 7c062aa Compare December 19, 2016 12:29
vicb
vicb reviewed Dec 19, 2016
View reviewed changes
modules/@angular/forms/src/directives/ng_novalidate_directive.ts Outdated
@Directive({
selector: 'form:not([ngNoForm])',
selector: 'form:not([ngNoForm]):not([ngNoNovalidate])',
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use ngNoNoValidate ?

Copy link
Contributor

Choose a reason for hiding this comment

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

... or ngNativeValidate ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose ngNoNoValidate to be consistent with ngNoForm and others. And I agree it sounds odd :)

ngNativeValidate

I really like your proposal.

@vicb
Copy link
Contributor

vicb commented Dec 19, 2016

@DzmitryShylovich when adding new features, could you please make sure to create a GH issue on angular/angular.io to ask for it to be documented on top of adding API docs to the source code ?

Thanks

@vicb
Copy link
Contributor

vicb commented Dec 19, 2016

Could you please also rename NgNovalidate to NgNoValidate (thanks @chuckjaz for catching this)

@DzmitryShylovich DzmitryShylovich changed the title feat(forms): add option to disable novalidate and still use angular forms feat(forms): add option to use native validation and angular forms Dec 19, 2016
@DzmitryShylovich
Copy link
Contributor Author

@vicb done

@vicb
Copy link
Contributor

vicb commented Dec 19, 2016

@DzmitryShylovich could you please mention the rename in the commit comment ? Have you opened an issue on angular/angular.io ?

@DzmitryShylovich
Copy link
Contributor Author

Have you opened an issue on angular/angular.io ?

#13573

@DzmitryShylovich
Copy link
Contributor Author

could you please mention the rename in the commit comment ?

done

@vicb vicb mentioned this pull request Dec 20, 2016
Open
@vicb vicb 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 Dec 20, 2016
@vicb
Copy link
Contributor

vicb commented Dec 20, 2016

The directive should be exported and the .d.ts updated

@DzmitryShylovich
Copy link
Contributor Author

@vicb why? It's a private api.

@DzmitryShylovich
Copy link
Contributor Author

@vicb I exported it

@mhevery
Copy link
Contributor

mhevery commented Jan 28, 2017

@kara can you approve this?

kara
kara approved these changes Feb 10, 2017
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM. Added a nit in case you want to fix.

modules/@angular/forms/src/directives/ng_no_validate_directive.ts Outdated
*
* `novalidate` is used to disable browser's native form validation.
*
* If you want to use native validation with angular forms just add `ngNativeValidate` attribute:
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: Can you add a comma? :)

If you want to use native validation with Angular forms, just add `ngNativeValidate` attribute:

@kara kara removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 10, 2017
@kara kara requested a review from IgorMinar February 10, 2017 20:08
@DzmitryShylovich
Copy link
Contributor Author

@kara done

@kara kara removed the request for review from IgorMinar February 11, 2017 00:01
@kara kara added the action: merge The PR is ready for merge by the caretaker label Feb 11, 2017
feat(forms): add option to use browser's native validation and angula…
a2de0f8 
…r forms

Also renames NgNovalidate -> NgNoValidate

Closes angular#13573
@vicb vicb merged commit 8742432 into angular:master Feb 21, 2017
@DzmitryShylovich DzmitryShylovich deleted the gh/no-novalidate branch February 21, 2017 06:55
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
@DzmitryShylovich
feat(forms): add option to use browser's native validation and angula…
038f474 
…r forms (angular#13566)

Also renames NgNovalidate -> NgNoValidate

Closes angular#13573
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
@DzmitryShylovich @juleskremer
feat(forms): add option to use browser's native validation and angula…
284a555 
…r forms (angular#13566)

Also renames NgNovalidate -> NgNoValidate

Closes angular#13573
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vicb vicb vicb left review comments

@kara kara kara approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: forms cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option to use browser's native validation and Angular forms
5 participants
@DzmitryShylovich @vicb @mhevery @kara @googlebot