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

exclude nested forms from validation #2632

Closed
wants to merge 1 commit into from
Closed

exclude nested forms from validation #2632

wants to merge 1 commit into from

Conversation

neelance
Copy link
Contributor

If a v-form contains a v-dialog which in turn contains another v-form, then it breaks the validation of the outer form, since inputs of the inner form affect the outer one.

Is this an acceptable solution to the issue?

If a v-form contains a v-dialog which in turn contains another v-form, then it breaks the validation of the outer form, since inputs of the inner form affect the outer one.
@codecov
Copy link

codecov bot commented Nov 24, 2017

Codecov Report

Merging #2632 into master will decrease coverage by 0.04%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2632      +/-   ##
==========================================
- Coverage   68.19%   68.14%   -0.05%     
==========================================
  Files         118      118              
  Lines        3147     3149       +2     
  Branches      996      997       +1     
==========================================
  Hits         2146     2146              
- Misses        716      718       +2     
  Partials      285      285
Impacted Files Coverage Δ
src/components/VForm/VForm.js 38.09% <50%> (+0.59%) ⬆️
src/components/VTimePicker/mixins/time-title.js 69.23% <0%> (-7.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb6f448...8beaed3. Read the comment docs.

@johnleider
Copy link
Member

Please do not push with --no-verify.

@neelance
Copy link
Contributor Author

I did not use any --no-verify.

@johnleider
Copy link
Member

The test is re-running, but it had previously failed with lint errors, you shouldn't be able to commit or push with test/lint errors without --no-verify.

@neelance
Copy link
Contributor Author

neelance commented Nov 24, 2017

I used the GitHub web UI for this small change. Did a full checkout now since I saw the linter errors.

@neelance
Copy link
Contributor Author

CI is happy now, but codecov isn't. Unfortunately I don't have the time to get to know your testing setup and a test case with a nested v-form and triggering validation seems non-trivial. What's your policy on this?

@KaelWD
Copy link
Member

KaelWD commented Nov 26, 2017

You can ignore that, it's thinking there's changes in VTimePicker for some reason.

@vuetifyjs vuetifyjs deleted a comment from codecov bot Nov 26, 2017
@neelance
Copy link
Contributor Author

Okay, thanks. So is this a good solution and ready to merge?

@nekosaur
Copy link
Member

nekosaur commented Nov 27, 2017

It's a solution for now. Forms will most likely be refactored to use inject/provide for 1.1. Although I am actually not sure how to solve this problem when we do..

edit: nevermind, it should be automatically solved by using inject/provide

@johnleider
Copy link
Member

Forms are currently being refactored to use provide/inject for 1.1. Was already mentioned here #2033. Thank you for your contribution.

@johnleider johnleider closed this Dec 3, 2017
@lock
Copy link

lock bot commented Apr 15, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. Please direct any non-bug questions to our Discord

@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants