-
Notifications
You must be signed in to change notification settings - Fork 4
Improve StrongForm validation UX and text input styling #1661
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
base: master
Are you sure you want to change the base?
Changes from all commits
e53f68a
4fc0c43
5eace68
036d512
8a8394a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -154,6 +154,8 @@ function AddServiceAccountForm(props: { | |
| }, | ||
| }); | ||
|
|
||
| const nicknameFieldProps = strongForm.getFieldComponentProps("nickname"); | ||
|
|
||
| const handleOpenChange = (nextOpen: boolean) => { | ||
| setOpen(nextOpen); | ||
| if (!nextOpen) { | ||
|
|
@@ -197,46 +199,47 @@ function AddServiceAccountForm(props: { | |
| </NewAlert> | ||
| </Modal.Body> | ||
| ) : ( | ||
| <Modal.Body> | ||
| <form | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, hmm, I'm not sure of the usage of the form elements with useStrongForm 🤔 But I'll check this out next time me work
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this could be achieved by attaching event listeners to inputs, but on a bigged form that's a lot of boilerplate. Maybe the getFieldComponentProps could now be used to avoid that? But then we'd need to be able to tell different field types apart (input vs textarea vs select...). Just a thought, some completely different approach might be better. |
||
| className="service-accounts__form" | ||
| onSubmit={(e) => { | ||
| e.preventDefault(); | ||
| if (strongForm.isReady) { | ||
| strongForm.submit(); | ||
| } | ||
| }} | ||
| > | ||
| <div> | ||
| Enter the nickname of the service account you wish to add to the | ||
| team <span>{props.teamName}</span> | ||
| </div> | ||
| <div className="service-accounts__nickname-input"> | ||
| <NewTextInput | ||
| value={formInputs.nickname} | ||
| onChange={(e) => { | ||
| updateFormFieldState({ | ||
| field: "nickname", | ||
| value: e.target.value, | ||
| }); | ||
| }} | ||
| placeholder={"ExampleName"} | ||
| maxLength={32} | ||
| /> | ||
| <div className="service-accounts__nickname-input-max-length"> | ||
| Max. 32 characters | ||
| <> | ||
| <Modal.Body> | ||
| <div className="service-accounts__form"> | ||
| <div> | ||
| Enter the nickname of the service account you wish to add to the | ||
| team <span>{props.teamName}</span> | ||
| </div> | ||
| <label htmlFor="serviceAccountNickname"> | ||
| Nickname <span aria-hidden="true">*</span> | ||
| </label> | ||
| <div className="service-accounts__nickname-input"> | ||
| <NewTextInput | ||
| id="serviceAccountNickname" | ||
| value={formInputs.nickname} | ||
| onChange={(e) => { | ||
| updateFormFieldState({ | ||
| field: "nickname", | ||
| value: e.target.value, | ||
| }); | ||
| }} | ||
| placeholder={"ExampleName"} | ||
| maxLength={32} | ||
| required | ||
| {...nicknameFieldProps} | ||
| /> | ||
| <div className="service-accounts__nickname-input-max-length"> | ||
| Max. 32 characters | ||
| </div> | ||
| </div> | ||
| {error && <NewAlert csVariant="danger">{error}</NewAlert>} | ||
| </div> | ||
| {error && <NewAlert csVariant="danger">{error}</NewAlert>} | ||
| </form> | ||
| </Modal.Body> | ||
| )} | ||
| {serviceAccountAdded ? null : ( | ||
| <Modal.Footer> | ||
| <NewButton onClick={strongForm.submit} disabled={!strongForm.isReady}> | ||
| Add Service Account | ||
| </NewButton> | ||
| </Modal.Footer> | ||
| </Modal.Body> | ||
| <Modal.Footer> | ||
| <NewButton | ||
| onClick={strongForm.submit} | ||
| disabled={!strongForm.isReady} | ||
| > | ||
| Add Service Account | ||
| </NewButton> | ||
| </Modal.Footer> | ||
| </> | ||
| )} | ||
| </Modal> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { useEffect, useMemo, useState } from "react"; | ||
| import { useCallback, useEffect, useMemo, useState } from "react"; | ||
| import { | ||
| ParseError, | ||
| RequestBodyParseError, | ||
|
|
@@ -57,20 +57,105 @@ export function useStrongForm< | |
| const [submitOutput, setSubmitOutput] = useState<SubmissionOutput>(); | ||
| const [submitError, setSubmitError] = useState<SubmissionError>(); | ||
| const [inputErrors, setInputErrors] = useState<InputErrors>(); | ||
| const [fieldInteractions, setFieldInteractions] = useState< | ||
| Partial< | ||
| Record< | ||
| keyof Inputs, | ||
| { | ||
| hasFocused: boolean; | ||
| hasBlurred: boolean; | ||
| } | ||
| > | ||
| > | ||
| >({}); | ||
| const [hasAttemptedSubmit, setHasAttemptedSubmit] = useState(false); | ||
|
|
||
| const isValueEmpty = (value: unknown) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the body of useStrongForm is getting rather long, so a helper function like this could be moved elsewhere. |
||
| if (typeof value === "string") { | ||
| return value.trim() === ""; | ||
| } | ||
| return value === undefined || value === null; | ||
| }; | ||
|
|
||
| const isReady = useMemo(() => { | ||
| if (!props.validators) return true; | ||
| for (const key in props.validators) { | ||
| const validator = props.validators[key]; | ||
| const value = props.inputs[key]; | ||
| const value = props.inputs[key as keyof Inputs]; | ||
| // NOTE: Expand the checks as more validators are added | ||
| if (validator?.required) { | ||
| if (typeof value === "string" && value.trim() === "") return false; | ||
| if (value === undefined || value === null) return false; | ||
| if (validator?.required && isValueEmpty(value)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| }, [props.inputs]); | ||
| }, [props.inputs, props.validators]); | ||
|
|
||
| const getFieldState = useCallback( | ||
| <K extends keyof Inputs>(field: K) => { | ||
| const validator = props.validators?.[field]; | ||
| const value = props.inputs[field]; | ||
| const isRequired = Boolean(validator?.required); | ||
| const rawInvalid = isRequired && isValueEmpty(value); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As part of my own task, I tried to build a URL validator for the team profile form on top of the changes on this PR. Having duplicate checks here and in |
||
| const interactions = fieldInteractions[field]; | ||
| const hasFinishedInteraction = | ||
| Boolean(interactions?.hasFocused && interactions?.hasBlurred) || | ||
| hasAttemptedSubmit; | ||
| const isInvalid = rawInvalid && hasFinishedInteraction; | ||
| return { | ||
| isRequired, | ||
| isInvalid, | ||
| }; | ||
| }, | ||
| [fieldInteractions, hasAttemptedSubmit, props.inputs, props.validators] | ||
| ); | ||
|
|
||
| const markFieldInteraction = useCallback( | ||
| (field: keyof Inputs, type: "focus" | "blur") => { | ||
| setFieldInteractions((prev) => { | ||
| const current = prev[field] ?? { hasFocused: false, hasBlurred: false }; | ||
| const next = | ||
| type === "focus" | ||
| ? { ...current, hasFocused: true } | ||
| : { ...current, hasBlurred: true }; | ||
| if ( | ||
| current.hasFocused === next.hasFocused && | ||
| current.hasBlurred === next.hasBlurred | ||
| ) { | ||
| return prev; | ||
| } | ||
| return { ...prev, [field]: next }; | ||
| }); | ||
| }, | ||
| [] | ||
| ); | ||
|
|
||
| const getFieldInteractionProps = useCallback( | ||
| (field: keyof Inputs) => ({ | ||
| onFocus: () => markFieldInteraction(field, "focus"), | ||
| onBlur: () => markFieldInteraction(field, "blur"), | ||
| }), | ||
| [markFieldInteraction] | ||
| ); | ||
|
|
||
| const getFieldComponentProps = useCallback( | ||
| (field: keyof Inputs, options?: { disabled?: boolean }) => { | ||
| const fieldState = getFieldState(field); | ||
| const modifiers: ("invalid" | "disabled")[] = []; | ||
| if (fieldState.isInvalid) { | ||
| modifiers.push("invalid" as const); | ||
| } | ||
| if (options?.disabled) { | ||
| modifiers.push("disabled" as const); | ||
| } | ||
| const interactionProps = getFieldInteractionProps(field); | ||
| return { | ||
| ...interactionProps, | ||
| "aria-invalid": fieldState.isInvalid, | ||
| csModifiers: modifiers, | ||
| }; | ||
| }, | ||
| [getFieldInteractionProps, getFieldState] | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (refining || submitting) { | ||
|
|
@@ -106,6 +191,7 @@ export function useStrongForm< | |
| }, [props.inputs]); | ||
|
|
||
| const submit = async () => { | ||
| setHasAttemptedSubmit(true); | ||
| if (submitting) { | ||
| const error = new Error("Form is already submitting!"); | ||
| if (props.onSubmitError) { | ||
|
|
@@ -193,5 +279,8 @@ export function useStrongForm< | |
| refineError, | ||
| inputErrors, | ||
| isReady, | ||
| getFieldState, | ||
| getFieldInteractionProps, | ||
| getFieldComponentProps, | ||
| }; | ||
| } | ||

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.
Thoughts on:
title="Required"to this element?