Conversation
RainerSchmoeger
requested review from
Copilot and
knime-ghub-bot
and removed request for
a team
🦋 Changeset detectedLatest commit: 1e98e89 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
Introduces initial fieldset support primitives in the KDS forms package and starts deprecating legacy “form primitive” exports (KdsLabel, KdsSubText) in favor of using form-field component props directly.
Changes:
- Added
KdsFieldsetPropstype and a new internalBaseFieldsetWrapperhelper component. - Marked
KdsLabel/KdsSubText(and their prop types) as deprecated informs/index.ts. - Updated the WAC→KDS migration guide to note
Label/SubTextas removed (internal-only).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/components/src/forms/types.ts | Adds new fieldset prop typing for internal fieldset wrapper usage. |
| packages/components/src/forms/index.ts | Deprecates public exports for internal label/subtext primitives. |
| packages/components/src/forms/_helper/BaseFieldsetWrapper.vue | Introduces a new fieldset wrapper helper (currently unused). |
| .github/agents/wac-to-kds-migrator.md | Updates migration mapping guidance for Label/SubText removals. |
RainerSchmoeger
force-pushed
the
KDS-489-fieldset
branch
from
727c9c6 to
dccaa66
Compare
…ubTextProps; update description in KdsFieldsetProps for clarity KDS-489 ()
|
| export type KdsFieldsetProps = { | ||
| /** | ||
| * Fieldset id | ||
| */ | ||
| id?: string; | ||
| /** | ||
| * Legend text displayed above the fieldset content. | ||
| */ | ||
| label?: string; | ||
| /** | ||
| * Accessible label used when no visible legend is rendered. | ||
| */ | ||
| ariaLabel?: string; | ||
| /** | ||
| * Optional description displayed in an info popover next to the legend. | ||
| * The info toggle button is only visible when hovering or focusing the fieldset. | ||
| */ | ||
| description?: string; | ||
| /** | ||
| * Override the implicit ARIA role of the fieldset (defaults to `group`). | ||
| * Use `radiogroup` for radio button groups. | ||
| */ | ||
| role?: string; | ||
| } & Omit<KdsSubTextProps, "id">; |
There was a problem hiding this comment.
KdsFieldsetProps allows both label and ariaLabel to be omitted, which makes it possible to render an unnamed <fieldset> (accessibility issue). Consider mirroring the existing KdsInputLabelProps pattern in this file by making label XOR ariaLabel (require one of them), so consumers can't accidentally create an unlabeled group.
| :aria-describedby="props.subText ? subTextId : undefined" | ||
| @mouseenter="isHovered = true" | ||
| @mouseleave="isHovered = false" | ||
| > |
There was a problem hiding this comment.
The JSDoc says the info toggle is visible when “hovering or focusing the fieldset”, but visibility is currently driven only by mouseenter/mouseleave (isHovered). Add focus-based handling (e.g. focusin/focusout or :focus-within) so keyboard users also get the description button as documented.
| :content="props.description" | ||
| :hidden="!isHovered" |
There was a problem hiding this comment.
KdsInfoToggleButton is always rendered when description is set, and :hidden="!isHovered" only makes it transparent. Since the button remains focusable in the DOM, users can tab to an invisible control. Consider hiding it in a way that removes it from the tab order (e.g. use v-show/v-if based on hover/focus state, or update the hidden mechanism to make the button non-focusable when hidden).
| :content="props.description" | |
| :hidden="!isHovered" | |
| v-show="isHovered" | |
| :content="props.description" |
| &:focus:not(.selected) { | ||
| border: var(--kds-border-action-selected); |
There was a problem hiding this comment.
The new focus styling uses border: var(--kds-border-action-selected) on :focus:not(.selected), which can make a focused-but-unselected item look selected. Consider using the focused token (e.g. --kds-border-action-focused) and/or applying the style on :focus-visible instead of :focus to avoid changing appearance on mouse focus.
| &:focus:not(.selected) { | |
| border: var(--kds-border-action-selected); | |
| &:focus-visible:not(.selected) { | |
| border: var(--kds-border-action-focused); |
| "@knime/kds-components": patch | ||
| --- | ||
|
|
||
| Deprecate use of internal KdsLabel and KdsSubtext |
There was a problem hiding this comment.
The changeset text uses inconsistent casing: KdsSubtext should be KdsSubText to match the component/type name used in the codebase and exports.
| Deprecate use of internal KdsLabel and KdsSubtext | |
| Deprecate use of internal KdsLabel and KdsSubText |
KDS-489 ()