-
Notifications
You must be signed in to change notification settings - Fork 592
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
handle validate for choices #4793
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just would like @boutell opinion on a point.
@@ -656,6 +673,9 @@ module.exports = { | |||
continue; | |||
} | |||
} | |||
if (isParentVisible === false) { | |||
continue; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boutell Would like your opinion on that:
In this file just before line 672
we add default value to hidden fields.
If this is necessary, I suppose that we should do the same thing when a parent is hidden?
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with val, not visible is not visible, so apply the same policy re: supplying a default but only if the value is not valid for the field already (e.g. reuse that code in both not-visible situations)
* main: fix vue warnings (#4797) Pro 6694 breakpoint preview vite ready (#4789) Resolve yaml dependency conflicts (#4801) PRO-6775: External frontend support, docs cleanup (#4799) sort glob result (#4796) HMR condition argument and widget player fix (#4794) fix permission grid tooltip display (#4792) Fix extra bundle detection (#4791) release 4.9.0 (#4788) Pro 6535 revision (#4787)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* main: fixes modals deep selectors (#4795)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Might be improved in the future.
Summary
Log only errors for visible fields + wait for choices to trigger validate
What are the specific steps to test this change?
For example:
What kind of change does this PR introduce?
(Check at least one)
Make sure the PR fulfills these requirements:
If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.
Other information: