fix(pds-radio): add contained variant and image slot #562
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
bug
QuintonJason
changed the title
QuintonJason
force-pushed
the
fix/radio-add-bordered-variant
branch
from
5a5e386 to
f39b2e1
Compare
|
@coderabbitai please review this PR |
|
@QuintonJason: I'll review the changes in this PR, which adds a contained variant and image slot to the ✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request extends the PdsRadio component with two new features: a Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
libs/core/src/components/pds-radio/pds-radio.scss (2)
83-95: Avoid duplicate visually-hidden definitions.Both
input.visually-hiddenand global.visually-hiddenimplement the same pattern. Keep one utility to reduce drift.- /* stylelint-disable-next-line selector-no-qualifying-type */ - &.visually-hidden { - border: 0; - clip: rect(0 0 0 0); - clip-path: polygon(0 0, 0 0, 0 0); - height: 1px; - margin: -1px; - overflow: hidden; - padding: 0; - position: absolute; - white-space: nowrap; - width: 1px; - } + /* Rely on the global `.visually-hidden` utility */Also applies to: 159-170
224-253:grid-areahas no effect without a grid container.This block uses
grid-areawhile the parent layout is flex. Remove or introduce a grid container where intended.libs/core/src/components/pds-radio/pds-radio.tsx (1)
101-109: Make image-slot detection reactive to slot changes.
querySelector('[slot="image"]')won’t trigger re-render when slot content changes. Track state viaslotchange.+import { Component, Host, h, Prop, Event, EventEmitter, Element, State } from '@stencil/core'; ... - private hasImageSlot(): boolean { + @State() private _hasImage = false; + + private hasImageSlot(): boolean { const imageSlot = this.el.querySelector('[slot="image"]'); return !!imageSlot; } private hasImage(): boolean { - return this.hasImageSlot(); + return this._hasImage; } + + componentWillLoad() { + this._hasImage = this.hasImageSlot(); + } ... - {this.hasImage() && ( + {this.hasImage() && ( <div class="pds-radio__image-container" part="image-container"> - <slot name="image" /> + <slot name="image" onSlotchange={() => (this._hasImage = this.hasImageSlot())} /> </div> )}Also applies to: 176-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
libs/core/src/components.d.ts(3 hunks)libs/core/src/components/pds-radio/docs/pds-radio.mdx(2 hunks)libs/core/src/components/pds-radio/pds-radio.scss(4 hunks)libs/core/src/components/pds-radio/pds-radio.tsx(6 hunks)libs/core/src/components/pds-radio/readme.md(2 hunks)libs/core/src/components/pds-radio/stories/pds-radio.stories.js(3 hunks)libs/core/src/components/pds-radio/test/pds-radio.e2e.ts(2 hunks)libs/core/src/components/pds-radio/test/pds-radio.spec.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
libs/core/src/components/pds-radio/pds-radio.tsx
[error] 136-136: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (12)
libs/core/src/components.d.ts (3)
1383-1387: Public prop added matches implementation.
hasBorder: booleanlooks good and consistent with TSX and docs.
3999-4002: JSX typings updated correctly.Optional
hasBorder?: booleanin LocalJSX is appropriate.
4025-4027: Event doc improvement acknowledged.Clarified
pdsRadioChangepayload. No issues.libs/core/src/components/pds-radio/pds-radio.scss (3)
50-53: Verify:has()support for your browser matrix.
:has()on:hostis modern. Confirm support for all target browsers or provide a fallback class.Would you like a lightweight JS fallback (toggle
is-checkedclass on change) if:hasis unavailable?Also applies to: 202-204
9-9: Host positioning looks good.
position: relative;enables the label overlay to capture clicks. No issues.
174-174: Typography update acknowledged.Switch to
var(--pine-typography-body)is fine.libs/core/src/components/pds-radio/test/pds-radio.spec.tsx (1)
191-244: Good coverage forhasBorder.Assertions cover static, dynamic, and combined states.
libs/core/src/components/pds-radio/docs/pds-radio.mdx (1)
85-95: New examples read well.Bordered and image-slot demos align with the API.
Also applies to: 122-145, 146-179
libs/core/src/components/pds-radio/stories/pds-radio.stories.js (1)
9-9: Story wiring forhasBorderis correct.Props map and example are good.
Also applies to: 32-33, 77-83
libs/core/src/components/pds-radio/readme.md (1)
16-16: README updates match the public API.Property, event payload, slot, and part docs look good.
Also applies to: 28-31, 33-38, 40-45
libs/core/src/components/pds-radio/test/pds-radio.e2e.ts (1)
62-117: E2E coverage forhas-borderlooks solid.Static, absence, toggle, and combo states validated.
libs/core/src/components/pds-radio/pds-radio.tsx (1)
5-8: API/docs annotations look good.Slot and part JSDoc plus
hasBorderprop are consistent.Also applies to: 46-51
| :host(.has-border) { | ||
| align-items: flex-start; | ||
| border: var(--pine-border); | ||
| border-radius: var(--pine-dimension-125); | ||
| column-gap: var(--pine-dimension-xs); | ||
| display: flex; | ||
| flex: 1; | ||
| flex-direction: column; | ||
| padding: var(--pine-dimension-md); | ||
|
|
||
| &:hover { | ||
| border: var(--pine-border-hover); | ||
| } | ||
|
|
||
| &:has(input:checked) { | ||
| box-shadow: 0 0 0 2px var(--pine-color-primary-hover); | ||
| } | ||
|
|
||
| label { | ||
| color: var(--pine-color-text); | ||
| font-weight: var(--pine-font-weight-semi-bold); | ||
|
|
||
| &::after { | ||
| content: ""; | ||
| cursor: pointer; | ||
| height: 100%; | ||
| left: 0; | ||
| position: absolute; | ||
| top: 0; | ||
| width: 100%; | ||
| z-index: 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add visible focus styles for keyboard users when input is visually hidden.
Currently, input:focus-visible won’t be perceptible in bordered/image variants. Mirror focus to the container/host.
:host(.has-border) {
+ /* keyboard focus */
+ &:has(input:focus-visible) {
+ outline: var(--pine-outline-focus);
+ outline-offset: 2px;
+ }
&:has(input:checked) {
box-shadow: 0 0 0 2px var(--pine-color-primary-hover);
}
}
:host(.has-image) {
+ /* keyboard focus */
+ &:has(input:focus-visible) {
+ outline: var(--pine-outline-focus);
+ outline-offset: 2px;
+ }
&:has(input:checked) {
box-shadow: 0 0 0 2px var(--pine-color-primary-hover);
}
}Also applies to: 189-206
🤖 Prompt for AI Agents
In libs/core/src/components/pds-radio/pds-radio.scss around lines 36 to 69 (and
also apply the same change to lines 189-206), the keyboard focus on the visually
hidden input isn't visible for bordered/image variants—mirror the input's
:focus-visible state to the host/container by adding a selector like
:host(.has-border):has(input:focus-visible) (and equivalent for the image
variant) that applies a clear visible focus style (outline or box-shadow and
optional border-color change) so keyboard users see focus even when the input
itself is visually hidden; ensure the focus style matches existing hover/checked
styles and respects design tokens (e.g., use var(--pine-color-primary-hover) or
a dedicated focus token).
pixelflips
left a comment
pixelflips
left a comment
There was a problem hiding this comment.
Non-blocker, but should we add an image story? Otherwise, LGTM! 👍🏼
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| > | ||
| <pds-icon icon={danger} size="small" /> | ||
| {this.errorMessage} | ||
| )} |
There was a problem hiding this comment.
Bug: Radio Component Fails to Detect Dynamic Image Content
The pds-radio component's image slot is conditionally rendered. If the component starts without slotted image content, the slot and its onSlotchange handler are not present. This prevents detection of dynamically added image content, as the _hasImage state cannot update.
Description
pds-radio- add contained variantimageslotFixes DSS-1473
Type of change
How Has This Been Tested?
Visit the Radio page, scroll to the With Image and Image Group section and verify
Test Configuration:
Checklist:
If not applicable, leave options unchecked.