Improve StrongForm validation UX and text input styling #1661
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR extends the StrongForm system with per-field APIs and integrates them across several settings forms. useStrongForm gains getFieldState, getFieldInteractionProps, and getFieldComponentProps, plus internal tracking for per-field interactions and submission attempts. Team name, username, and service-account nickname inputs are wired to these per-field props, made controlled, and marked required; submission flows were adjusted to use strongForm.submit where applicable. Theme CSS adds invalid-state tokens and input/button state refinements to surface validation states in the UI. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cyberstorm-remix/app/settings/teams/Teams.tsx (1)
234-236: Modal may close on submit error.
strongForm.submit()can throw an error, but.then(() => setOpen(false))will still execute if the promise resolves (even whenonSubmitErroris called internally before re-throwing). Consider usingasync/awaitwith try/catch, or movesetOpen(false)intoonSubmitSuccessto ensure the modal stays open on error.- onClick={() => { - strongForm.submit().then(() => setOpen(false)); - }} + onClick={async () => { + try { + await strongForm.submit(); + setOpen(false); + } catch { + // Modal stays open on error + } + }}
🧹 Nitpick comments (3)
packages/cyberstorm-theme/src/components/TextInput/TextInput.css (1)
31-33: Wrapper invalid icon color missing focus-within state.Line 31-33 sets the icon color for invalid state, but line 27-29 still applies focus color unconditionally. When an invalid input is focused, the icon will show focus color instead of invalid color.
Consider updating to maintain invalid styling during focus:
&:focus-within .text-input__left-icon { + &:not(.text-input__wrapper--invalid) { --text-input-left-icon-color: var(--input-icon-color--focus); + } } &.text-input__wrapper--invalid .text-input__left-icon { --text-input-left-icon-color: var(--input-icon-color--invalid); }apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (2)
73-78: Consider extendingisValueEmptyfor arrays/objects.Currently handles strings,
undefined, andnull. If you later need to validate that an array has items or an object has keys, this helper would incorrectly consider[]or{}as non-empty.const isValueEmpty = (value: unknown) => { if (typeof value === "string") { return value.trim() === ""; } + if (Array.isArray(value)) { + return value.length === 0; + } return value === undefined || value === null; };
191-191: Missing dependencies in useEffect dependency array.The effect references
props.refiner,props.onRefineSuccess, andprops.onRefineError(lines 172, 174-175, 182-183) but the dependency array only includesprops.inputs. If these callbacks change between renders, the effect will use stale versions, potentially causing bugs.Add them to the dependency array or wrap them with
useRefto maintain stable references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/cyberstorm-remix/app/settings/teams/Teams.tsx(3 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MemberAddForm.tsx(3 hunks)apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx(2 hunks)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts(4 hunks)packages/cyberstorm-theme/src/components/TextInput/TextInput.css(2 hunks)packages/cyberstorm-theme/src/components/componentsColors.css(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx (1)
packages/cyberstorm/src/newComponents/Modal/Modal.tsx (1)
Modal(185-345)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (9)
apps/cyberstorm-remix/app/settings/teams/team/tabs/Members/MemberAddForm.tsx (1)
98-99: LGTM!Clean integration of the new
getFieldComponentPropshelper. The spread placement after explicit props is correct, and thearia-hidden="true"on the asterisk appropriately hides the visual indicator from screen readers while therequiredattribute handles accessibility.Also applies to: 126-127, 141-142
apps/cyberstorm-remix/app/settings/teams/Teams.tsx (1)
186-187: LGTM!Consistent use of the new field props pattern. Required indicator and accessibility attributes are properly configured.
Also applies to: 213-213, 216-216, 225-226
packages/cyberstorm-theme/src/components/TextInput/TextInput.css (1)
62-79: CSS specificity: hover/focus may override invalid styling.The invalid state rules (lines 62-68) are placed before hover/focus rules (lines 70-79). Since both use equivalent specificity, hover and focus styles will override the invalid styling when users interact with an invalid input.
If the intent is for invalid styling to persist during hover/focus, consider either:
- Adding
:not(.text-input--invalid)to the hover/focus rules, or- Repeating the invalid rules after hover/focus to ensure they take precedence
.text-input:hover { + &:not(.text-input--invalid) { --text-input-background-color: var(--input-bg-color--hover); --text-input-border-color: var(--input-border-color--hover); + } } .text-input:focus-within { + &:not(.text-input--invalid) { --text-input-text-color: var(--input-text-color--focus); --text-input-background-color: var(--input-bg-color--focus); --text-input-border-color: var(--input-border-color--focus); + } }packages/cyberstorm-theme/src/components/componentsColors.css (1)
266-266: LGTM!New invalid state color tokens follow existing naming conventions and use appropriate red accent colors for visual feedback.
Also applies to: 270-270, 274-274, 279-279
apps/cyberstorm-remix/app/settings/teams/team/tabs/ServiceAccounts/ServiceAccounts.tsx (1)
157-158: LGTM!Clean refactor to use Modal subcomponents with the new field props pattern. The direct
onClick={strongForm.submit}approach avoids the modal-close-on-error issue seen in Teams.tsx. Consistent integration with the rest of the PR.Also applies to: 202-242
apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts (4)
1-1: LGTM!Good additions for field interaction tracking. The
fieldInteractionsstate structure andhasAttemptedSubmitflag enable the interaction-aware validation UX described in the PR objectives.Also applies to: 60-71
93-110: Well-designed field state logic.The interaction-aware validation (
hasFinishedInteractioncombining focus+blur or submit attempt) provides good UX by not showing errors before user engagement. Clean implementation.
112-130: Nice optimization with idempotent updates.The early return when state hasn't changed (lines 120-125) prevents unnecessary re-renders. Good practice.
140-158: Clean composition of field props.
getFieldComponentPropsnicely combines state derivation with interaction handlers and ARIA attributes. The optionaldisabledparameter provides flexibility.One consideration:
csModifiersreturns a new array on each call. IfNewTextInputdoes shallow comparison oncsModifiers, this could cause re-renders. Memoizing the array or using a stable reference pattern would help if this becomes an issue.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## testing_setup #1661 +/- ##
=================================================
- Coverage 11.57% 11.51% -0.07%
=================================================
Files 320 321 +1
Lines 22915 23023 +108
Branches 509 509
=================================================
- Hits 2652 2650 -2
- Misses 20263 20373 +110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| <div className="add-member-form__field add-member-form__username"> | ||
| <label className="add-member-form__label" htmlFor="username"> | ||
| Username | ||
| Username <span aria-hidden="true">*</span> |
There was a problem hiding this comment.
Thoughts on:
- adding something like
title="Required"to this element? - having a dedicated component for it to not have to repeat this over and over around the project?
| </Modal.Body> | ||
| ) : ( | ||
| <Modal.Body> | ||
| <form |
There was a problem hiding this comment.
The form was here on purpose to allow submitting the form with enter on text input, as is expected. It shouldn't be removed as part of these changes.
The changes also makes the modal layout awkward:
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| >({}); | ||
| const [hasAttemptedSubmit, setHasAttemptedSubmit] = useState(false); | ||
|
|
||
| const isValueEmpty = (value: unknown) => { |
There was a problem hiding this comment.
Nit: the body of useStrongForm is getting rather long, so a helper function like this could be moved elsewhere.
| const validator = props.validators?.[field]; | ||
| const value = props.inputs[field]; | ||
| const isRequired = Boolean(validator?.required); | ||
| const rawInvalid = isRequired && isValueEmpty(value); |
There was a problem hiding this comment.
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 isReady gets less ideal with each validator that is added. Maybe check if that can be avoided?
- add interaction-aware helpers to `useStrongForm` so inputs only go invalid after user focus/blur or submit, and expose ready-to-spread props for focus/blur/csModifiers/aria - refactor create-team, add-member, and add-service-account forms to use the new helper props instead of hand-rolled wiring - teach the theme’s TextInput component about invalid state colors so `csModifiers` visually reflect errors
Backend requires owner-level permissions so no point waste other member's time by letting them fiddle with the form. I considered creating entirely separate component for viewing the data, which would save us from having to set up strongForm for no reason, but given there's no reusable components, it would repeat a lot of markup and lead to poor maintainability.
Change the on-hover cursor when to indicate the input is disabled.
Remove pointer-events: none rule from disabled buttons so they show the default on-hover cursor for them.
Oksamies
force-pushed
the
12-09-improve_strongform_validation_ux_and_text_input_styling
branch
from
8a8394a to
cec3894
Compare
Oksamies
force-pushed
the
graphite-base/1661
branch
from
2cbec7e to
de89cb7
Compare
useStrongFormso inputs only go invalid after user focus/blur or submit, and expose ready-to-spread props for focus/blur/csModifiers/ariacsModifiersvisually reflect errors