-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[SECURITY_SOLUTION][ENDPOINT] Trusted App Create Form show inline validations errors #78305
[SECURITY_SOLUTION][ENDPOINT] Trusted App Create Form show inline validations errors #78305
Conversation
…ted-app-create-form-validations
Pinging @elastic/endpoint-management (Team:Endpoint Management) |
Pinging @elastic/endpoint-app-team (Feature:Endpoint) |
...ugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_page.test.tsx
Show resolved
Hide resolved
LGTM, just looks like some test and type errors |
Thanks @kevinlog . yes, the CI errors were expected and should now (that I pushed the tests) be addressed. |
…ted-app-create-form-validations # Conflicts: # x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/__snapshots__/trusted_apps_page.test.tsx.snap
const [formValues, setFormValues] = useState<NewTrustedApp>({ | ||
name: '', | ||
os: 'windows', | ||
entries: [generateNewEntry()], | ||
description: '', | ||
}); | ||
|
||
const [validationResult, setValidationResult] = useState<ValidationResult>(() => |
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.
when are we using react's state vs redux state?
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.
😝 answer here 👉 https://www.elastic.co/about/our-source-code#it-depends
😄 to expand on my reasoning:
For this one - the entire thing - it was developed as a "pure" component and it is not tied to the Store. I did not see the benefit of storing this in Redux while the data is being collected from the user. Another reason, but one that really would probably not have convinced me to change the approach, was that at the time I started it, the trusted apps store setup was not yet available (pending PR).
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
💚 Build SucceededMetrics [docs]async chunks size
History
To update your PR or re-run it, just comment with: |
…idations errors (elastic#78305) * Updated structure for `ValidationResult` type * show errors on the ui if field is invalid * Support for tracking visited fields * Remove use of Snapshots in Trusted Apps tests # Conflicts: # x-pack/plugins/security_solution/public/management/pages/trusted_apps/view/__snapshots__/trusted_apps_page.test.tsx.snap
…idations errors (#78305) (#78449) * Updated structure for `ValidationResult` type * show errors on the ui if field is invalid * Support for tracking visited fields * Remove use of Snapshots in Trusted Apps tests Co-authored-by: Elastic Machine <[email protected]>
Summary
Display form validation errors inline with the appropriate form fields. Validation are only displayed after the user has"visited" (clicked into/out of) a field. Required fields are also marked as such and thus display the EUI required field indicator (red bottom border).
In addition: this PR also removes use of snapshot tests in Trusted Apps page view
Checklist
Delete any items that are not applicable to this PR.