Refactor(web-react): Standalone Label, HelperText & ValidationText #DS-2398

[crishpeen]

Description

Additional context

Issue reference

[netlify]

✅ Deploy Preview for spirit-design-system canceled.

Name	Link
🔨 Latest commit	9c34369
🔍 Latest deploy log	https://app.netlify.com/projects/spirit-design-system/deploys/69c50d299d93710008cf5115

[github-actions]

Here is the URL of the uploaded artifact: https://github.com/alma-oss/spirit-design-system/actions/runs/23202889982/artifacts/5968123307

[github-actions]

Here is the URL of the uploaded artifact: https://github.com/alma-oss/spirit-design-system/actions/runs/23202890306/artifacts/5968128753

[crishpeen]

when #2548 is merged, the e2e should be ok

[github-actions]

Here is the URL of the uploaded artifact: https://github.com/alma-oss/spirit-design-system/actions/runs/23204858220/artifacts/5969035406

[Copilot]

Pull request overview

This PR refactors form-field subcomponents in web-react by extracting Label, HelperText, and ValidationText into standalone components (instead of being bundled under Field), and updates the web (vanilla) docs/demos to use the new “default Label” styling (no Label--box).

Changes:

• Introduces standalone Label, HelperText, and ValidationText components in packages/web-react and wires them into existing form controls via PropsProvider.
• Updates multiple vanilla packages/web component demos/READMEs to use class="Label" (and related modifiers) consistently.
• Removes the Field subpath export and related types/tests, and adds supporting hooks/types (useAriaIds, FormFieldVariants, shared form-field context types).

Reviewed changes

Copilot reviewed 135 out of 143 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/web/src/scss/components/**	Updates HTML demos/READMEs to use Label base class instead of Label--box and adjusts related markup formatting.
packages/web/src/scss/components/Label/_Label.scss	Removes the explicit “box variant” selector and makes “box” the default (non-inline, non-item).
packages/web-react/src/components/**	Replaces Field/* usage with standalone Label, HelperText, ValidationText and introduces styling hooks for these components.
packages/web-react/src/context/PropsContext.ts	Filters undefined from provider values to avoid unintentionally overwriting parent context.
packages/web-react/package.json	Removes ./components/Field export from the package export map.

Comments suppressed due to low confidence (1)

packages/web-react/package.json:328

• The package export map removes the ./components/Field subpath export, but there are no new subpath exports for the new standalone components (./components/Label, ./components/HelperText, ./components/ValidationText). Given the pattern used for other components, this likely makes the new standalone components unavailable via subpath imports and introduces a breaking change for existing .../components/Field consumers. Consider adding explicit exports for the new component subpaths (and/or a compatibility re-export for Field) and ensuring this is handled as a documented breaking change.

      "require": "./dist/components/EmptyState/index.cjs",
      "default": "./dist/components/EmptyState/index.js"
    },
    "./components/FieldGroup": {
      "types": "./dist/components/FieldGroup/index.d.ts",
      "development": "./src/components/FieldGroup/index.ts",
      "production": "./dist/components/FieldGroup/index.js",
      "import": "./dist/components/FieldGroup/index.js",
      "require": "./dist/components/FieldGroup/index.cjs",
      "default": "./dist/components/FieldGroup/index.js"
    },

[netlify]

✅ Deploy Preview for spirit-design-system-storybook canceled.

Name	Link
🔨 Latest commit	9c34369
🔍 Latest deploy log	https://app.netlify.com/projects/spirit-design-system-storybook/deploys/69c50d2959ce8800089c2923

[github-actions]

Here is the URL of the uploaded artifact: https://github.com/alma-oss/spirit-design-system/actions/runs/23301127719/artifacts/6008408670

[crishpeen]

I updated the PR and tried to resolve all your comments; hopefully, I didn't miss anything or resolve any unresolved issues. If it happened, I'm sorry, and let me know.

[dlouhak]

Significant: Could you please add stories for Label and ValidationText?
Nitpicking: A few comments about query selectors. Looks like we should write down the rules for writing unit tests for coding agents. 🙈

[github-actions]

Here is the URL of the uploaded artifact: https://github.com/alma-oss/spirit-design-system/actions/runs/23482434335/artifacts/6076851956

[github-actions]

Here is the URL of the uploaded artifact: https://github.com/alma-oss/spirit-design-system/actions/runs/23485099826/artifacts/6077992458

[github-actions]

Here is the URL of the uploaded artifact: https://github.com/alma-oss/spirit-design-system/actions/runs/23497208260/artifacts/6083340977

[cursor]

Security review update: I re-validated the current diff and checked prior unresolved review threads; no unresolved security findings remain applicable.

I reviewed only modified code paths and traced attacker-controlled inputs to sinks in the new standalone Label, HelperText, and ValidationText integrations (PropsProvider propagation, aria registration, and rendering in form components). I did not find any medium/high/critical vulnerability introduced by this PR:

• No new HTML/JS injection sink (dangerouslySetInnerHTML, eval, dynamic script construction) was introduced.
• User-provided label/helper/validation content remains rendered through React nodes (framework escaping applies for string content).
• No auth/authz boundary changes, request-forgery surfaces, SSRF/path traversal/file-system sinks, or secret logging paths were introduced in changed runtime code.
• No dependency/supply-chain risk was introduced by this diff.

Inline finding comments: none (no high-confidence security vulnerability).

Slack summary: PR #2547 security review complete — no medium/high/critical vulnerabilities found in modified code paths; no active security findings to address.

  

_{Sent by Cursor Automation: Find vulnerabilities}

[cursor]

Stale comment

Code Review: refactor/DS-2398-react-standalone-form-fields

Scope: 12 commits, 165 files (+7410 / −2723)

Overview

1. Splits Label, HelperText, and ValidationText out of the removed Field package into first-class components with their own package.json subpath exports and barrel exports.
2. Moves useAriaIds into hooks, adds focused tests, and wires form controls (TextFieldBase, Checkbox, Select, Slider, Toggle, FieldGroup, file uploaders, etc.) to the standalone pieces plus PropsProvider for shared field context.
3. Tightens PropsProvider merging so explicit null / undefined values remove keys from inherited context (documented in JSDoc).
4. Carries related SCSS demo/README updates, dropdown placement default handling, and refreshed visual snapshots.

---

1. packages/web-react/package.json — Subpath exports

suggestion (non-blocking): Document removal of @alma-oss/spirit-web-react/components/Field and map consumers to @alma-oss/spirit-web-react/components/Label, HelperText, and ValidationText (or the main package barrel).

Fix: Add a short entry to the next release notes / migration doc (e.g. docs/migrations/web-react/ or CHANGELOG.md) with before/after import examples.

Why: The ./components/Field export entry was removed; anyone using that deep import will get a resolution failure on upgrade. Calling it out avoids silent breakage during version bumps.

---

Summary

Finding type	Count
issue	0
suggestion	1
question	0
note	0

Verdict: APPROVE

No blocking issues; the refactor is coherent, tests and snapshots were updated, and accessibility registration for helper/validation text is preserved (with a small improvement: cleanup no longer calls remove when id is missing).

---

praise: Solid extraction: shared FormFieldContextValue typing, PropsProvider + useContextProps precedence documented, dedicated stories/demos for the new components, and broad test updates across inputs and provider tests—good regression safety for a structural change like this.

  

_{Sent by Cursor Automation: Spirit Cursor Code Review}

[github-actions]

Here is the URL of the uploaded artifact: https://github.com/alma-oss/spirit-design-system/actions/runs/23584370096/artifacts/6118857987

[cursor]

Stale comment

Code Review: refactor/DS-2398-react-standalone-form-fields

Scope: 15 commits, 168 files (+7376 / −2731)

Overview

1. Extracts Label, HelperText, and ValidationText as standalone components with dedicated style hooks, demos, stories, and tests.
2. Removes the old Field subcomponents and wires form controls (TextFieldBase, Checkbox, Radio, Select, Slider, Toggle, FieldGroup, FileUploader, UNSTABLE_FileUpload, etc.) to compose these pieces with PropsProvider and useAriaIds for aria-describedby.
3. Moves useAriaIds to shared hooks and expands formFieldContextPropsTest for consistent context vs. direct-prop behavior.
4. Updates Dropdown so useDropdownContext always yields a defined placement, and stops setting data-spirit-placement from useDropdownAriaProps (placement still drives classes via useDropdownStyleProps).
5. Refreshes web SCSS demos/docs and visual snapshots for the new composition patterns.

Summary

Category	Count
Blocking	0
Non-blocking	2
Questions	1
Praise	1

Findings by file

1. packages/web-react/src/components/ValidationText/ValidationText.tsx — List rendering

issue (non-blocking): Array validationText items use key={\validationText_${item}`}`, which collides when two entries share the same string.

File:

spirit-design-system/packages/web-react/src/components/ValidationText/ValidationText.tsx

Lines 70 to 74 in 4ce7aa2

	{Array.isArray(validationText) ? (
	<ul>
	{validationText.map((item) => (
	<li key={`validationText_${item}`}>{item}</li>
	))}

Fix: Use the index (or a stable id if messages can be reordered) as part of the key, e.g. key={\validationText_${index}_${String(item)}`}`.

Why: Duplicate messages produce duplicate React keys, which can cause incorrect reconciliation and confusing updates.

---

2. packages/web-react/src/hooks/useAriaIds.ts — External aria-describedby

suggestion (non-blocking): otherAriaIds is only read on the initial render; if a parent passes a new aria-describedby string later, internal state does not resync.

File:

spirit-design-system/packages/web-react/src/hooks/useAriaIds.ts

Lines 6 to 8 in 4ce7aa2

	export const useAriaIds = (otherAriaIds?: string): [string[], RegisterType] => {
	const [ids, setIds] = useState<string[]>(otherAriaIds ? otherAriaIds.split(' ') : []);

Fix: Consider syncing when otherAriaIds changes, e.g. useEffect(() => { setIds(otherAriaIds ? otherAriaIds.split(' ') : []); }, [otherAriaIds]); — only if controlled updates to aria-describedby are in scope; otherwise document that the hook treats the initial string as static.

Why: Avoids stale aria-describedby when the prop is driven by parent state.

---

3. packages/web-react/src/components/Dropdown/useDropdownAriaProps.ts — data-spirit-placement

question (non-blocking): data-spirit-placement was removed from popover contentProps; placement classes still come from useDropdownStyleProps.

File:

spirit-design-system/packages/web-react/src/components/Dropdown/useDropdownAriaProps.ts

Lines 35 to 48 in 4ce7aa2

	}
	export const useDropdownAriaProps = (props: UseDropdownAriaPropsProps): UseDropdownAriaPropsReturn => {
	const { fullWidthMode, id, isOpen, toggleHandler } = props;
	const triggerProps = {
	[NAME_ARIA_EXPANDED]: isOpen,
	[NAME_ARIA_CONTROLS]: String(id),
	onClick: toggleHandler,
	};
	const contentProps = {
	id,
	[NAME_DATA_FULLWIDTHMODE]: fullWidthMode,
	};

Why: Confirm this matches product/docs expectations for scripts or tests that read data-spirit-placement on the React-rendered popover.

---

Verdict

COMMENT — One non-blocking issue (list keys); no blocking findings.

---

praise: Strong refactor: clear separation of Label / HelperText / ValidationText, consistent PropsProvider + useAriaIds wiring across inputs, thorough formFieldContextPropsTest coverage, and good README/demo alignment for consumers building custom fields.

  

_{Sent by Cursor Automation: Spirit Cursor Code Review}

[cursor]

Code Review: refactor/DS-2398-react-standalone-form-fields

Scope: 16 commits, 168 files (+7519 / −2731)

Overview

1. Extracts Label, HelperText, and ValidationText into standalone components with style hooks, demos, stories, and package exports; removes the Field barrel and ./components/Field export map entry.
2. Moves useAriaIds to hooks/, wires registerAria from form controls into helper/validation text for aria-describedby, and adds shared formFieldContextPropsTest coverage.
3. Updates PropsProvider merge semantics so nested providers can clear inherited keys (e.g. null), with JSDoc aligned to direct-prop precedence.
4. Adjusts Dropdown so useDropdownContext always exposes placement (with default) and stops duplicating placement on the popover via data-spirit-placement (placement remains on the styled popover path).

Summary

Severity	Count
Blocking	0
Non-blocking	2
Questions	0
Notes	1

Verdict: APPROVE

No blocking findings. One suggestion on list item keys in ValidationText (duplicate string messages) and a note on documenting the Field removal for consumers. Nice work on tests and context precedence coverage.

---

Full review (Conventional Comments, file-grouped)

See automation output / thread for grouped findings with permalinks.

  

_{Sent by Cursor Automation: Spirit Cursor Code Review}