feat(pds-radio-group) add pds-radio-group to system #601
Conversation
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
bot
added
package: core
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a new Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
libs/core/src/components/pds-radio/stories/pds-radio.stories.js (1)
30-30: Removeindeterminate— radios don't support this property.The
indeterminateattribute is specific to checkboxes; radio buttons don't have an indeterminate state. This appears to be leftover from copying a checkbox template.name=${args.name} - ?indeterminate=${args.indeterminate} ?required=${args.required}libs/core/src/components/pds-radio-group/test/pds-radio-group.e2e.ts (1)
85-88: Consider scoping selectors through the host element for shadow‑DOM robustnessFor error/helper message queries, consider patterns like
page.find('pds-radio-group >>> .pds-radio-group__message--error')andpage.find('pds-radio-group >>> .pds-radio-group__message:not(.pds-radio-group__message--error)')to avoid accidentally matching unrelated elements and to be resilient if the component uses shadow DOM.Also applies to: 118-121
libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx (1)
232-268: Tests reaching into internals could be brittle over timeA few tests call private methods (
(component as any).updateChildRadios()) and overridepdsRadioGroupChange.emitbefore invokinghandleRadioChangedirectly. This gives precise control but also couples the tests to internals, so refactors (method rename, event wiring changes) may break tests without changing external behavior. If the test harness ever supports reliably triggeringslotchangeand native change events end‑to‑end, consider moving these to more black‑box interactions.Also applies to: 270-393
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (2)
172-174: Consider using a more deterministic ID generation strategy.The fallback ID generation using
Math.random()on Line 173 works for client-side rendering but produces non-deterministic IDs that could complicate SSR scenarios or snapshot testing.Consider using a deterministic counter-based approach:
+let radioGroupIdCounter = 0; + export class PdsRadioGroup { @Element() el: HTMLPdsRadioGroupElement; render() { - const groupId = this.componentId || `radio-group-${Math.random().toString(36).substr(2, 9)}`; + const groupId = this.componentId || `radio-group-${++radioGroupIdCounter}`; const gapValue = this.getGapValue();
61-64: Consider validating or documenting the name prop requirement.The
nameprop is essential for proper radio group behavior (mutual exclusivity and form submission), but it has no default value or validation. Ifnameis not provided, child radios may not function correctly as a group.Consider either:
- Add validation in componentDidLoad:
componentDidLoad() { if (!this.name) { console.warn('pds-radio-group: name prop is required for proper radio group functionality'); } this.updateChildRadios(); }
- Or add clear documentation in the JSDoc comment:
/** - * String used for radio `name` attribute. Applied to all child radios. + * String used for radio `name` attribute. Applied to all child radios. + * Required for proper radio group behavior (mutual exclusivity and form submission). */ @Prop() name: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
libs/core/src/components.d.ts(9 hunks)libs/core/src/components/pds-radio-group/docs/pds-radio-group.mdx(1 hunks)libs/core/src/components/pds-radio-group/pds-radio-group.scss(1 hunks)libs/core/src/components/pds-radio-group/pds-radio-group.tsx(1 hunks)libs/core/src/components/pds-radio-group/radio-group-interface.ts(1 hunks)libs/core/src/components/pds-radio-group/readme.md(1 hunks)libs/core/src/components/pds-radio-group/stories/pds-radio-group.stories.js(1 hunks)libs/core/src/components/pds-radio-group/test/pds-radio-group.e2e.ts(1 hunks)libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx(1 hunks)libs/core/src/components/pds-radio/docs/pds-radio.mdx(2 hunks)libs/core/src/components/pds-radio/pds-radio.scss(5 hunks)libs/core/src/components/pds-radio/pds-radio.tsx(1 hunks)libs/core/src/components/pds-radio/stories/pds-radio.stories.js(1 hunks)libs/core/src/components/pds-radio/test/pds-radio.spec.tsx(1 hunks)libs/react/src/components/proxies.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
libs/core/src/components/pds-radio-group/radio-group-interface.ts (1)
libs/core/src/components.d.ts (1)
RadioGroupChangeEventDetail(25-25)
libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx (2)
libs/react/src/components/proxies.ts (2)
PdsRadioGroup(84-84)PdsRadio(83-83)libs/core/src/components.d.ts (1)
Event(19-19)
libs/core/src/components/pds-radio/test/pds-radio.spec.tsx (2)
libs/react/src/components/proxies.ts (2)
PdsRadio(83-83)PdsRadioGroup(84-84)libs/core/src/components.d.ts (1)
Event(19-19)
libs/react/src/components/proxies.ts (2)
libs/react/src/components/react-component-lib/createComponent.tsx (1)
createReactComponent(14-106)libs/react/src/components/react-component-lib/index.ts (1)
createReactComponent(1-1)
🪛 LanguageTool
libs/core/src/components/pds-radio-group/readme.md
[style] ~14-~14: ‘whether or not’ might be wordy. Consider a shorter alternative.
Context: ...d |disabled` | Determines whether or not the radio group is disabled. ...
(EN_WORDINESS_PREMIUM_WHETHER_OR_NOT)
[style] ~19-~19: ‘whether or not’ might be wordy. Consider a shorter alternative.
Context: ... |invalid` | Determines whether or not the radio group is invalid. ...
(EN_WORDINESS_PREMIUM_WHETHER_OR_NOT)
[style] ~21-~21: ‘whether or not’ might be wordy. Consider a shorter alternative.
Context: ...d |required` | Determines whether or not the radio group is required. ...
(EN_WORDINESS_PREMIUM_WHETHER_OR_NOT)
🔇 Additional comments (13)
libs/core/src/components/pds-radio/pds-radio.tsx (1)
100-104: LGTM — clean event delegation pattern.Using
closest()to detect the parent group and suppress the individual event is an efficient approach. This correctly delegates event responsibility to the group component.libs/core/src/components/pds-radio/test/pds-radio.spec.tsx (1)
191-211: LGTM — good negative test for grouped radio behavior.The test correctly verifies that individual
pdsRadioChangeevents are suppressed when the radio is inside apds-radio-group. Usingpage.doc.querySelectoris appropriate here since the root element is the group, not the radio.libs/core/src/components/pds-radio-group/radio-group-interface.ts (1)
1-5: LGTM — well-defined event detail interface.The interface correctly captures the essential information for radio group change events. Making
valueandcomponentIdoptional is appropriate since child radios may not always have these attributes set.libs/core/src/components/pds-radio-group/readme.md (1)
1-53: Auto-generated documentation looks complete.The property descriptions, event details, and dependency graph accurately reflect the component's public API.
libs/core/src/components/pds-radio/stories/pds-radio.stories.js (1)
16-16: LGTM — organizing radios under the Radio Group category.Nesting the Radio story under "Radio Group" in Storybook improves discoverability and aligns with the new component structure.
libs/core/src/components/pds-radio-group/test/pds-radio-group.e2e.ts (1)
3-338: Comprehensive e2e coverage for group behaviorThis suite exercises the key behaviors (prop propagation, single selection, messages, form submission, events, disabled/required handling) in realistic DOM contexts and looks solid overall.
libs/core/src/components/pds-radio-group/pds-radio-group.scss (1)
1-46: Styles align with group layout and validation modelThe SCSS cleanly separates host layout, row/column variants, and message/error styling using existing tokens. Constraining
.is-invalidto message color fits the group‑level error model.libs/react/src/components/proxies.ts (1)
36-36: React proxy forpds-radio-groupis wired consistentlyThe
definePdsRadioGroupimport andPdsRadioGroupexport match the established proxy pattern (types, tag name, defineCustomElement), so React consumers should be able to use the new group without extra setup.Also applies to: 84-84
libs/core/src/components/pds-radio-group/docs/pds-radio-group.mdx (1)
9-307: Docs are consistent with the new group API and usage patternsThe MDX page clearly explains when to use the group, matches prop names (
groupLabel,direction,helperMessage,errorMessage, etc.), and keeps React and web‑component examples in sync with the tests and stories.libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx (1)
5-394: Thorough spec coverage for core behaviorsThese specs exercise attribute propagation, dynamic prop updates, label/helper/error rendering, direction classes, slot changes, and event payloads in good detail, and they line up with the component’s public contract.
libs/core/src/components/pds-radio/pds-radio.scss (1)
12-18: Radio styles now align with group-driven validation and disabled statesConfining
.is-invalidto the error message (for both standard, bordered, and image variants) and adding explicit disabled styling for bordered/image radios makes the visuals consistent with the new radio‑group behavior while keeping token usage intact.Also applies to: 20-72, 156-166, 180-192, 194-293
libs/core/src/components/pds-radio-group/stories/pds-radio-group.stories.js (1)
3-133: Storybook stories exercise key configurations and look consistentThe base, bordered, image, and horizontal stories cover the primary layout and state combinations for
pds-radio-group, and the arg bindings match the documented API (groupLabel,direction, helper/error messages, disabled/invalid/required).libs/core/src/components/pds-radio-group/pds-radio-group.tsx (1)
85-107: This pattern is necessary and intentional; no changes required.The body-level event listener is the standard Stencil approach for container components that need to respond to events from their children. The investigation confirms:
- Necessity:
pds-radiocomponents render native inputs in their scoped shadow DOM. Change events from these inputs bubble to the light DOM wherepds-radio-grouplistens. Direct host-element listeners cannot capture events across the shadow DOM boundary.- Precedent: Multiple other Pine components use the same pattern (
pds-tabs,pds-buttonon body;pds-dropdown-menu,pds-filterson window/document).- Efficiency: The filtering logic (line 92: type check and containment validation) is robust and prevents handling unrelated form events. Only emitting on checked state (line 97) further reduces overhead.
- No alternatives: Stencil doesn't provide a cleaner way to listen to child component events across the shadow DOM boundary.
This is a sound implementation pattern with no performance concerns for typical applications.
|
@coderabbitai i have addressed issues and nitpicks. please review again |
|
@pixelflips: Thank you for addressing the previous feedback! I'll review the changes now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (1)
17-20: Consider using a more robust ID generation approach.The static counter increments indefinitely without reset. For long-running applications, consider using
crypto.randomUUID()or a similar approach instead of a counter, which would also eliminate the need to maintain state across instances.- /** - * Instance counter for deterministic ID generation - */ - private static radioGroupIdCounter = 0;Then in the render method:
- const groupId = this.componentId || `radio-group-${++PdsRadioGroup.radioGroupIdCounter}`; + const groupId = this.componentId || `radio-group-${crypto.randomUUID()}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/core/src/components.d.ts(9 hunks)libs/core/src/components/pds-radio-group/pds-radio-group.tsx(1 hunks)libs/core/src/components/pds-radio-group/readme.md(1 hunks)libs/core/src/components/pds-radio-group/test/pds-radio-group.e2e.ts(1 hunks)libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx(1 hunks)libs/core/src/components/pds-radio/docs/pds-radio.mdx(2 hunks)libs/core/src/components/pds-radio/stories/pds-radio.stories.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/core/src/components/pds-radio/docs/pds-radio.mdx
- libs/core/src/components/pds-radio-group/test/pds-radio-group.e2e.ts
- libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (4)
libs/react/src/components/proxies.ts (1)
PdsRadioGroup(84-84)libs/core/src/components.d.ts (2)
Event(19-19)RadioGroupChangeEventDetail(25-25)libs/core/src/components/pds-radio-group/radio-group-interface.ts (1)
RadioGroupChangeEventDetail(1-5)libs/core/src/utils/form.ts (1)
messageId(4-6)
🪛 LanguageTool
libs/core/src/components/pds-radio-group/readme.md
[style] ~14-~14: ‘whether or not’ might be wordy. Consider a shorter alternative.
Context: ...d |disabled` | Determines whether or not the radio group is disabled. ...
(EN_WORDINESS_PREMIUM_WHETHER_OR_NOT)
[style] ~19-~19: ‘whether or not’ might be wordy. Consider a shorter alternative.
Context: ... |invalid` | Determines whether or not the radio group is invalid. ...
(EN_WORDINESS_PREMIUM_WHETHER_OR_NOT)
[style] ~21-~21: ‘whether or not’ might be wordy. Consider a shorter alternative.
Context: ...d |required` | Determines whether or not the radio group is required. ...
(EN_WORDINESS_PREMIUM_WHETHER_OR_NOT)
🔇 Additional comments (5)
libs/core/src/components/pds-radio/stories/pds-radio.stories.js (1)
16-16: LGTM: Story organization improved.The updated title nests the Radio component under Radio Group in Storybook, improving navigation and reflecting the relationship between standalone radios and the new radio group component.
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (4)
115-137: LGTM: Proper attribute propagation for web components.The method correctly uses
setAttributeandremoveAttributeto propagate boolean and string attributes to child radio elements, which is the appropriate pattern for web component composition.
139-144: LGTM: Helpful validation warning.The warning for missing
nameprop provides helpful feedback to developers, and callingupdateChildRadiosensures initial synchronization with child elements.
159-179: LGTM: Gap value mapping implemented correctly.The method provides a clean mapping from t-shirt sizes to design tokens with a fallback for custom dimension tokens. The implementation follows common design system patterns.
181-226: LGTM: Well-structured render method with proper accessibility.The render implementation is clean and accessible:
- Proper use of conditional rendering
- Good semantic structure
- Aria-live region for error messages
- Slot change handler ensures child radio synchronization
|
@coderabbitai i have addressed the additional issues and nitpicks, please review again. |
|
@pixelflips: Thank you for addressing the additional feedback! I'll review the latest changes now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx (1)
1-405: Good test coverage overall; consider adding tests forgapprop.The test suite thoroughly covers prop propagation, event emission, and dynamic updates. However, there's no test coverage for the
gapprop and its CSS variable application. Consider adding a test to verify the gap functionality.it('applies correct gap CSS variable when gap prop is set', async () => { const page = await newSpecPage({ components: [PdsRadioGroup], html: `<pds-radio-group gap="lg"></pds-radio-group>`, }); const style = page.root?.style.getPropertyValue('--pds-radio-group-gap'); expect(style).toBe('var(--pine-dimension-lg)'); }); it('handles numeric gap values as dimension tokens', async () => { const page = await newSpecPage({ components: [PdsRadioGroup], html: `<pds-radio-group gap="100"></pds-radio-group>`, }); const style = page.root?.style.getPropertyValue('--pds-radio-group-gap'); expect(style).toBe('var(--pine-dimension-100)'); });libs/core/src/components/pds-radio-group/pds-radio-group.tsx (1)
202-218: Consider linking messages to child radios for accessibility.The helper and error messages have IDs but aren't linked to the radio inputs via
aria-describedby. For screen reader users, consider propagating these message IDs to the child radios or addingrole="group"witharia-describedbyon the Host.return ( <Host class={this.classNames()} id={this.groupId} + role="group" + aria-describedby={this.helperMessage || this.errorMessage + ? `${this.errorMessage ? messageId(this.groupId, 'error') : ''} ${this.helperMessage ? messageId(this.groupId, 'helper') : ''}`.trim() + : undefined} style={{ '--pds-radio-group-gap': gapValue, }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx(1 hunks)libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx (2)
libs/react/src/components/proxies.ts (2)
PdsRadioGroup(84-84)PdsRadio(83-83)libs/core/src/components.d.ts (1)
Event(19-19)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (4)
libs/core/src/components.d.ts (2)
Event(19-19)RadioGroupChangeEventDetail(25-25)libs/react/src/components/react-component-lib/interfaces.ts (1)
EventEmitter(2-4)libs/core/src/components/pds-radio-group/radio-group-interface.ts (1)
RadioGroupChangeEventDetail(1-5)libs/core/src/utils/form.ts (1)
messageId(4-6)
🔇 Additional comments (1)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (1)
86-106: Previous feedback addressed; listener is now scoped correctly.The
@Listen('change')decorator is now properly scoped to the component host, addressing the prior review feedback about removingtarget: 'body'.
|
@coderabbitai i have addressed the additional issues and nitpicks, please review again. |
|
@pixelflips: Thank you for addressing the additional feedback! I'll review the latest changes now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (3)
92-112: MakehandleRadioChangeresilient to shadow DOM/event retargeting
handleRadioChangeassumesevent.targetis anHTMLInputElementwithtype === 'radio'. This is correct today ifpds-radiorenders its<input>in the light DOM, but will break ifpds-radiois ever switched to use a shadow root (standard event retargeting would makeevent.targetthe host element, not the input).To make this handler future‑proof and more robust, consider resolving the input via
event.composedPath()instead ofevent.target:@Listen('change') handleRadioChange(event: Event) { - const target = event.target as HTMLInputElement; - - // Only handle events from radio inputs - if (target.type !== 'radio') { - return; - } + const path = (event.composedPath?.() || []) as EventTarget[]; + const target = path.find( + (node): node is HTMLInputElement => + node instanceof HTMLInputElement && node.type === 'radio' + ); + if (!target) { + return; + } // Only emit if the radio is being checked (not unchecked) if (target.checked) { const radioElement = target.closest('pds-radio') as HTMLPdsRadioElement; if (radioElement) { this.pdsRadioGroupChange.emit({ checked: true, value: radioElement.value, componentId: radioElement.componentId, }); } } }This keeps behavior the same now but avoids tight coupling to the current DOM structure of
pds-radio.
20-27: Stable group ID looks good; consider reacting to dynamiccomponentIdchangesUsing
_groupIdcomputed once incomponentWillLoadand wiring it throughidandmessageId()solves the previous unstable‑ID problem and is a solid approach.If you expect
componentIdto change at runtime (e.g., via a React wrapper prop update), you may want to mirror it into_groupIdvia a watcher to keep the hostidand ARIA message IDs in sync:@Watch('componentId') handleComponentIdChange(newValue: string | undefined) { if (newValue) { this._groupId = newValue; } }If
componentIdis considered immutable after initial render, the current implementation is fine and this can be skipped.Also applies to: 138-150, 208-221
162-169: Improve group-level a11y semantics (role and labeling)Right now the host uses
role="group"andgroupLabelis only rendered visually. For a radio set, a more semantic pattern would be:
- Use
role="radiogroup"on the host; and- Associate the visual label via
aria-labelledby(oraria-label) so AT users hear the group label when navigating options.For example:
- render() { + render() { const gapValue = this.getGapValue(); const ariaDescribedBy = this.getAriaDescribedBy(); + const labelId = this.groupLabel ? `${this._groupId}__label` : undefined; return ( <Host class={this.classNames()} id={this._groupId} - role="group" + role="radiogroup" aria-describedby={ariaDescribedBy} + aria-labelledby={labelId} style={{ '--pds-radio-group-gap': gapValue, }} > {this.groupLabel && ( - <div class="pds-radio-group__label"> + <div class="pds-radio-group__label" id={labelId}> {this.groupLabel} </div> )}This aligns the semantics with WAI‑ARIA patterns for radio groups and better ties helper/error messaging to the control cluster.
Also applies to: 208-251
libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx (1)
5-425: Comprehensive coverage; consider reducing white-box coupling when test harness allowsThe suite does a nice job exercising:
- Prop propagation (
name,invalid,required,disabled) including dynamic changes.- Rendering of label/helper/error (with icon) and direction classes.
- Gap token handling and numeric token fallback.
- Event emission semantics for
pdsRadioGroupChange.A couple of tests reach into internals (
(component as any).updateChildRadios()and direct calls tohandleRadioChange). Given the documented limitations of the Stencil test harness, this is reasonable for now, but once nativechange/slotchangebehavior is better supported, it would be good to refactor these to black‑box interactions so the tests are less coupled to private methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx(1 hunks)libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx (2)
libs/react/src/components/proxies.ts (2)
PdsRadioGroup(84-84)PdsRadio(83-83)libs/core/src/components.d.ts (1)
Event(19-19)
⏰ 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). (3)
- GitHub Check: test-specs (18)
- GitHub Check: test-specs (20)
- GitHub Check: test-specs (22)
🔇 Additional comments (1)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (1)
172-192: Gap token mapping and CSS variable usage look solid
getGapValuecleanly maps t‑shirt sizes to dimension tokens and falls back tovar(--pine-dimension-${gap})for numeric tokens, and the result is applied via--pds-radio-group-gapinline style. This is a good, flexible pattern for spacing tokens and matches the stated API.Also applies to: 218-220
|
@coderabbitai i have addressed the additional issues, please review again. |
|
@pixelflips: Thank you for addressing the additional feedback! I'll review the latest changes now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (3)
48-48: Consider using a union type for thegapprop.The
gapprop currently accepts any string, but the JSDoc indicates specific valid values. Using a union type would provide compile-time type safety and better IntelliSense support for developers.Consider this approach:
+type GapSize = 'none' | 'xxs' | 'xs' | 'sm' | 'md' | 'lg' | 'xl' | 'xxl' | '025' | '050' | '100' | '200' | '300' | '400' | '500' | '600' | '700' | '800'; + - @Prop() gap: string = 'sm'; + @Prop() gap: GapSize = 'sm';This prevents invalid values at compile time and improves the developer experience.
174-194: Consider adding validation or fallback for invalidgapvalues.The method generates CSS variable references without validating whether the
gapvalue is actually valid. Invalid values (e.g., typos) will produce non-existent CSS variables likevar(--pine-dimension-typo), causing the gap to fail silently.Consider adding a fallback to the default value:
private getGapValue(): string { const gapMap: Record<string, string> = { none: '0', xxs: 'var(--pine-dimension-xxs)', xs: 'var(--pine-dimension-xs)', sm: 'var(--pine-dimension-sm)', md: 'var(--pine-dimension-md)', lg: 'var(--pine-dimension-lg)', xl: 'var(--pine-dimension-xl)', xxl: 'var(--pine-dimension-xxl)', }; if (gapMap[this.gap]) { return gapMap[this.gap]; } + // Validate dimension token format (e.g., '025', '100', '200') + if (!/^\d{3}$/.test(this.gap)) { + console.warn(`pds-radio-group: Invalid gap value "${this.gap}", falling back to "sm"`); + return gapMap.sm; + } + return `var(--pine-dimension-${this.gap})`; }
122-136: Optional: Consider boolean attribute convention.The code sets boolean attributes with a string value
'true'(e.g.,setAttribute('disabled', 'true')). While this works, HTML boolean attributes conventionally use presence/absence or empty string. However, for Stencil components with prop reflection, the current approach may be intentional and consistent with howpds-radioexpects these attributes.If aligning with HTML conventions is desired:
if (this.disabled) { - radio.setAttribute('disabled', 'true'); + radio.setAttribute('disabled', ''); } else { radio.removeAttribute('disabled'); } if (this.invalid) { - radio.setAttribute('invalid', 'true'); + radio.setAttribute('invalid', ''); } else { radio.removeAttribute('invalid'); } if (this.required) { - radio.setAttribute('required', 'true'); + radio.setAttribute('required', ''); } else { radio.removeAttribute('required'); }However, verify that
pds-radiohandles empty-string attributes correctly before making this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx(1 hunks)libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/core/src/components/pds-radio-group/test/pds-radio-group.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (4)
libs/react/src/components/proxies.ts (1)
PdsRadioGroup(84-84)libs/core/src/components.d.ts (2)
Event(19-19)RadioGroupChangeEventDetail(25-25)libs/core/src/components/pds-radio-group/radio-group-interface.ts (1)
RadioGroupChangeEventDetail(1-5)libs/core/src/utils/form.ts (1)
messageId(4-6)
🔇 Additional comments (2)
libs/core/src/components/pds-radio-group/pds-radio-group.tsx (2)
1-258: Excellent component implementation with strong accessibility foundations.The component follows Stencil best practices with proper lifecycle management, stable ID generation, scoped event handling, and comprehensive accessibility support (role="group", aria-describedby, aria-live). The slot change handler ensures dynamically added radios receive group attributes, and past review concerns have been properly addressed.
20-20: Verify whether dynamiccomponentIdchanges should be supported.Currently,
_groupIdis generated once incomponentWillLoadbased on the initialcomponentIdvalue. IfcomponentIdchanges after initialization,_groupIdwill not update, potentially breaking accessibility relationships with message IDs.If dynamic
componentIdupdates are expected, add a@Watch('componentId')handler to update_groupId. If IDs are meant to be set once (typical pattern), the current implementation is correct.If dynamic updates are needed, apply this pattern:
+ @Watch('componentId') + handleComponentIdChange() { + if (this.componentId) { + this._groupId = this.componentId; + } + }Also applies to: 140-152
… listener to host
…ds for accessibility
pixelflips
force-pushed
the
feat/pds-radio-group
branch
from
6063449 to
8cf3429
Compare
|
@amylu555 you can use the preview link above in the netlify comment to take a look at the new radio group component. |
Description
This PR introduces a new
pds-radio-groupcomponent that wraps multiplepds-radiocomponents and provides group-level functionality, including a single error message for the entire group. This aligns with the new Figma design and simplifies form management for grouped radio selections.Key Features
name,disabled,invalid, andrequiredattributes to all child radiosdirectionprop to arrange radios horizontally (row) or vertically (column)hasBorder), and radios with imagespdsRadioGroupChangewhen a radio in the group is selecteddisabledandinvalidstates that propagate to all radiosChanges Made
pds-radio-groupcomponent with full TypeScript definitions and React wrapperspds-radioto suppress individual events when inside apds-radio-group(prevents duplicate events)pds-radiodocumentation with technical notes about usingpds-radio-grouppds-radiostoriesTechnical Details
pds-radiousage is unaffectedNo new dependencies or updates required.
Fixes DSS-24
Type of change
How Has This Been Tested?
Test Configuration:
Test Coverage
name,disabled,invalid,required)pds-radiotests continue to passChecklist: