feat(combinator): DLT-3312 preview resolved value in sized props#1199
feat(combinator): DLT-3312 preview resolved value in sized props#1199Francis Rupert (francisrupert) wants to merge 20 commits intonextfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a token resolution system that allows component props to display resolved CSS custom property values in the combinator UI. It refactors variant handling to use a top-level Changes
Sequence DiagramsequenceDiagram
participant User as User Interaction
participant Combinator as Combinator Component
participant MemberGroup as Option Bar Member Group
participant Control as Control Component
participant ExclusionLib as Exclusion Rules Lib
participant TokenLib as Token Resolver Lib
participant UI as Rendered UI
User->>Combinator: Select variant
Combinator->>Combinator: mergeVariantData() with defaults
Combinator->>MemberGroup: Render members with variant props
MemberGroup->>ExclusionLib: getDisabledValues(prop, rules, values)
ExclusionLib-->>MemberGroup: disabled values Set
MemberGroup->>Control: Pass tokenCategory, propValues, disabledValues
Control->>Control: Map options with computed resolved field
Control->>TokenLib: resolveTokenValue(category, value, propValues)
TokenLib->>TokenLib: Measure CSS custom properties / compute styles
TokenLib-->>Control: Resolved display string (e.g., "16px / 1.5")
Control->>UI: Render option with tooltip, swatch, or resolved text
UI-->>User: Display resolved token value & disabled state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Francis Rupert (francisrupert)
added
the
no-visual-test
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/combinator/src/components/option_bar/option_bar_member_group.vue (1)
20-29:⚠️ Potential issue | 🟠 MajorAdd regression tests for the new
disabledValues/propValuescontrol wiring.Line 20 through Line 29 introduces behavior that conditionally disables specific options at render time. This is non-trivial UI logic and should have test coverage (rule match, rule miss, and token resolution for enabled options) before merge.
As per coding guidelines: "Flag missing tests for non-trivial logic changes — untested behavior is unverified behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/combinator/src/components/option_bar/option_bar_member_group.vue` around lines 20 - 29, Add regression tests covering the new disabledValues/propValues wiring introduced around getDisabledValues in option_bar_member_group.vue: write component tests that (1) pass propValues and exclusionRules to assert getDisabledValues correctly disables matching options (rule match), (2) assert options remain enabled when rules don't apply (rule miss), and (3) verify tokenCategory/token resolution still yields enabled options when appropriate; mount the component (or the child that receives :args) with controlled members/values and assert rendered option elements' disabled state and that propValues are respected. Ensure tests call the same props (propValues, exclusionRules) and inspect the disabledValues prop passed into the child to cover the new wiring.
🧹 Nitpick comments (1)
packages/combinator/src/components/controls/control_selection.vue (1)
50-51: Addaria-disabledfor screen reader announcement of disabled items.The click handler guard correctly prevents selection for both mouse and keyboard input. However, disabled items lack
aria-disabled="true", so screen readers won't announce the disabled state to users. Consider adding it for better accessibility.<dt-list-item v-for="option in options" :key="option.value" role="menuitem" navigation-type="arrow-keys" :class="{ 'd-o50 d-pe-none': option.disabled }" + :aria-disabled="option.disabled || undefined" `@click`="!option.disabled && (onInput(option.value), close())" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/combinator/src/components/controls/control_selection.vue` around lines 50 - 51, The option element toggles visual disabled state via option.disabled but lacks an accessibility attribute; update the element that currently uses :class and `@click` (the same node referencing option.disabled and calling onInput(option.value) and close()) to also set aria-disabled so screen readers announce the state—for example bind aria-disabled to option.disabled (e.g., :aria-disabled="option.disabled ? 'true' : 'false'") so when option.disabled is true the control exposes aria-disabled="true".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/combinator/src/lib/exclusion_rules.js`:
- Around line 33-47: Add unit tests for getDisabledValues that cover: 1) no
exclusionRules returns an empty Set; 2) when conditions that are functions
(predicate) and plain equality both match and result in disabled values being
returned; 3) multiple rules combine (union) disabled values across rules; and 4)
disabled values are coerced to strings (e.g., numbers/booleans become
"1"/"true"). Use the getDisabledValues function directly, pass representative
exclusionRules and propValues, and assert the Set contents and string coercion
behavior for each case.
---
Outside diff comments:
In `@packages/combinator/src/components/option_bar/option_bar_member_group.vue`:
- Around line 20-29: Add regression tests covering the new
disabledValues/propValues wiring introduced around getDisabledValues in
option_bar_member_group.vue: write component tests that (1) pass propValues and
exclusionRules to assert getDisabledValues correctly disables matching options
(rule match), (2) assert options remain enabled when rules don't apply (rule
miss), and (3) verify tokenCategory/token resolution still yields enabled
options when appropriate; mount the component (or the child that receives :args)
with controlled members/values and assert rendered option elements' disabled
state and that propValues are respected. Ensure tests call the same props
(propValues, exclusionRules) and inspect the disabledValues prop passed into the
child to cover the new wiring.
---
Nitpick comments:
In `@packages/combinator/src/components/controls/control_selection.vue`:
- Around line 50-51: The option element toggles visual disabled state via
option.disabled but lacks an accessibility attribute; update the element that
currently uses :class and `@click` (the same node referencing option.disabled and
calling onInput(option.value) and close()) to also set aria-disabled so screen
readers announce the state—for example bind aria-disabled to option.disabled
(e.g., :aria-disabled="option.disabled ? 'true' : 'false'") so when
option.disabled is true the control exposes aria-disabled="true".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 10cce13e-2099-47bd-879d-f9153299569d
📒 Files selected for processing (21)
packages/combinator/src/components/combinator.vuepackages/combinator/src/components/controls/control_segmented.vuepackages/combinator/src/components/controls/control_selection.vuepackages/combinator/src/components/option_bar/option_bar_member_group.vuepackages/combinator/src/components/renderer/renderer_target.vuepackages/combinator/src/lib/exclusion_rules.jspackages/combinator/src/lib/tokens.jspackages/combinator/src/variants/variants_avatar.jspackages/combinator/src/variants/variants_badge.jspackages/combinator/src/variants/variants_checkbox.jspackages/combinator/src/variants/variants_description_list.jspackages/combinator/src/variants/variants_emoji.jspackages/combinator/src/variants/variants_emoji_text_wrapper.jspackages/combinator/src/variants/variants_icon.jspackages/combinator/src/variants/variants_input.jspackages/combinator/src/variants/variants_loader.jspackages/combinator/src/variants/variants_progress_circle.jspackages/combinator/src/variants/variants_radio.jspackages/combinator/src/variants/variants_select_menu.jspackages/combinator/src/variants/variants_stack.jspackages/combinator/src/variants/variants_text.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/combinator/src/lib/tokens.js (1)
84-86: Optional: Guard against missingkindOverrideforcomponent-size.If
categoryis'component-size'without the:componentNamesuffix,kindOverridewill beundefined, producing classd-undefined d-undefined--size-.... Returnsnullgracefully but silently fails.Defensive check
case 'component-size': + if (!kindOverride) { + result = null; + break; + } result = resolveComponentSize(kindOverride, value); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/combinator/src/lib/tokens.js` around lines 84 - 86, The 'component-size' branch calls resolveComponentSize(kindOverride, value) without checking kindOverride, producing invalid classes when kindOverride is undefined; update the case in tokens.js to guard for missing kindOverride (the variable from the parsed token/category) and return null (or otherwise short-circuit) when kindOverride is falsy so resolveComponentSize is only called with a valid component name; mention and change the behavior where tokens.js handles 'component-size' and ensure consumers of resolveComponentSize can handle a null return gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/combinator/src/lib/tokens.js`:
- Around line 84-86: The 'component-size' branch calls
resolveComponentSize(kindOverride, value) without checking kindOverride,
producing invalid classes when kindOverride is undefined; update the case in
tokens.js to guard for missing kindOverride (the variable from the parsed
token/category) and return null (or otherwise short-circuit) when kindOverride
is falsy so resolveComponentSize is only called with a valid component name;
mention and change the behavior where tokens.js handles 'component-size' and
ensure consumers of resolveComponentSize can handle a null return gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 16911e56-28f8-4179-92fb-52bc5be3e0c5
📒 Files selected for processing (2)
packages/combinator/src/lib/exclusion_rules.jspackages/combinator/src/lib/tokens.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/combinator/src/lib/exclusion_rules.js
|
Addressing remaining CodeRabbit findings: Regression tests for Guard |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/combinator/src/lib/tokens.js`:
- Around line 62-158: The PR adds DOM-dependent token resolution and caching but
lacks tests; add focused unit tests for resolveTokenValue covering each branch
(spacing via resolveSpacing, icon-size via resolveIconSize, typography-size
including propValues.kind and the 'typography-size:label' kindOverride that
calls resolveTypographySize, line-height via resolveLineHeight, and
component-size via resolveComponentSize) plus cache behavior (call twice to
ensure cached result returned, and clear/reset cache between tests). In tests,
set up a DOM environment (jsdom) or mock
getMeasureElement/getComputedStyle/resolveCssVar to return predictable values
for width/fontSize/lineHeight and verify outputs (including formatted "px" and
"font / line-height" ratio), and include negative cases (missing CSS var,
auto/0px sizes) to assert null returns. Ensure tests reference resolveTokenValue
and, where needed, stub getMeasureElement and the module-level cache to isolate
and reset state between test cases.
- Around line 15-20: The measurement div created in getMeasureElement
(measureEl) can affect page overflow and trigger scrollbars; update the
element's styling when you create it (and where else measureEl is used) to move
it fully offscreen and prevent layout impact—e.g., make it position:fixed with
left:-9999px (or top:-9999px), set width:0, height:0, overflow:hidden and
pointer-events:none/visibility:hidden so it cannot inflate scrollWidth or cause
flashes; ensure these style changes are applied wherever measureEl is
initialized or adjusted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 25044c81-6172-4fd3-bc35-1668df4aabc7
📒 Files selected for processing (3)
packages/combinator/src/components/controls/control_selection.vuepackages/combinator/src/lib/exclusion_rules.test.jspackages/combinator/src/lib/tokens.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/combinator/src/components/controls/control_selection.vue
Francis Rupert (francisrupert)
requested review from
Brad Paugh (braddialpad),
Ignacio Ropolo (iropolo) and
Nina Repetto (ninarepetto)
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5bd9d0f2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Changing back to draft as I want to add a related feature for color: https://dialpad.atlassian.net/browse/DLT-3313 |
…nd CSS property types
…nd CSS property types
… add color token variants for badge decoration, button link kind, and split button kind
|
Out of draft now. In addition to the original intent of this PR, previewing rendered dimensions in Prop Panel, I added color swatches. PR description updated, along with additional screenshots and video. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65320c83f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
CodeRabbit (@coderabbitai) review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/combinator/src/lib/tokens.js`:
- Around line 63-64: The swatch cache key currently ignores the active avatar
variant causing family swatches to resolve against variant "3" and become stale;
update the cache key construction (the cacheKey variable used where
cache.has(cacheKey) / cache.get(cacheKey) are called) to include the active
variant value (thread through the variant argument used for resolving
data-avatar-family), or if the active variant is unavailable, bypass returning a
cached swatch (i.e., skip swatch generation). Apply the same change to the other
cache checks at the locations analogous to lines 87-88 and 119-120 so the
variant is consistently accounted for or the swatch is skipped when variant is
unknown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: cc951c08-080e-4eaa-b4ad-bea9ec73cb43
📒 Files selected for processing (15)
packages/combinator/src/components/combinator.vuepackages/combinator/src/components/controls/control_selection.vuepackages/combinator/src/lib/tokens.jspackages/combinator/src/variants/variants_avatar.jspackages/combinator/src/variants/variants_badge.jspackages/combinator/src/variants/variants_button.jspackages/combinator/src/variants/variants_link.jspackages/combinator/src/variants/variants_notice.jspackages/combinator/src/variants/variants_presence.jspackages/combinator/src/variants/variants_progress_circle.jspackages/combinator/src/variants/variants_split_button.jspackages/combinator/src/variants/variants_text.jspackages/combinator/src/variants/variants_toast.jspackages/dialtone-vue/components/button/button.vuepackages/dialtone-vue/components/split_button/split_button.vue
💤 Files with no reviewable changes (1)
- packages/dialtone-vue/components/button/button.vue
✅ Files skipped from review due to trivial changes (4)
- packages/combinator/src/variants/variants_notice.js
- packages/combinator/src/variants/variants_toast.js
- packages/combinator/src/variants/variants_link.js
- packages/dialtone-vue/components/split_button/split_button.vue
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/combinator/src/variants/variants_badge.js
- packages/combinator/src/variants/variants_progress_circle.js
- packages/combinator/src/components/combinator.vue
- packages/combinator/src/variants/variants_text.js
- packages/combinator/src/components/controls/control_selection.vue
| const cacheKey = `${category}:${value}:${propValues?.kind ?? ''}`; | ||
| if (cache.has(cacheKey)) return cache.get(cacheKey); |
There was a problem hiding this comment.
Make avatar family swatches use the active avatar variant.
variants_avatar.js now enables family color previews while variant is still selectable, but this path always resolves data-avatar-family against data-avatar-variant="3" and the cache key only varies by kind. That makes the swatch wrong for any non-3 avatar variant, and once cached it stays wrong across variant changes. Please thread variant through the color resolver/cache here, or skip the swatch when the active variant is unknown.
Also applies to: 87-88, 119-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/combinator/src/lib/tokens.js` around lines 63 - 64, The swatch cache
key currently ignores the active avatar variant causing family swatches to
resolve against variant "3" and become stale; update the cache key construction
(the cacheKey variable used where cache.has(cacheKey) / cache.get(cacheKey) are
called) to include the active variant value (thread through the variant argument
used for resolving data-avatar-family), or if the active variant is unavailable,
bypass returning a cached swatch (i.e., skip swatch generation). Apply the same
change to the other cache checks at the locations analogous to lines 87-88 and
119-120 so the variant is consistently accounted for or the swatch is skipped
when variant is unknown.
There was a problem hiding this comment.
This is by design. The swatch previews the hue family, not the exact shade and rendering of the Avatar. The variant prop controls lightness/chroma of that family selection, and threading it through the resolver and cache would add significant complexity for a subtle distinction.
The swatch provides a representative mid-range color for each of the 12 hue families, which is all the swatch needs to communicate.
There was a problem hiding this comment.
@francisrupert That makes sense — using variant 3 as a representative mid-range hue sample is a reasonable design decision for a family-level color swatch. Withdrawing the concern.
✏️ Learnings added
Learnt from: francisrupert
Repo: dialpad/dialtone PR: 1199
File: packages/combinator/src/lib/tokens.js:63-64
Timestamp: 2026-04-15T19:43:12.302Z
Learning: In dialpad/dialtone's Combinator (`packages/combinator/src/lib/tokens.js`), the avatar `family` color swatch intentionally resolves against a hardcoded `data-avatar-variant="3"` to represent the mid-range hue for each of the 12 color families. The `variant` prop controls lightness/chroma within a family, not the hue itself, so it is deliberately excluded from the swatch resolver and cache key. Threading `variant` through would add complexity without meaningful benefit to the swatch's purpose.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: francisrupert
Repo: dialpad/dialtone PR: 1194
File: packages/dialtone-css/lib/build/less/components/notice.less:55-59
Timestamp: 2026-04-12T21:13:39.883Z
Learning: Repo: dialpad/dialtone — PR `#1194`. For CSS-internal refactors (e.g., notice/banner/toast moving from descendant selectors to CSS custom property inheritance) that produce identical compiled CSS, maintainers consider this non-behavioral and do not require new tests; a brief compiled-CSS diff or verification note in the PR is acceptable proof.
Learnt from: braddialpad
Repo: dialpad/dialtone PR: 1177
File: packages/dialtone-vue/components/checkbox/checkbox_constants.js:1-5
Timestamp: 2026-04-09T22:23:30.222Z
Learning: In the dialtone repo (PR `#1177`), the broad rename from `error`/`success`/`danger` to `critical`/`positive` across component prop values, CSS class names, and constants is an intentional breaking change with no legacy fallback aliases desired. This applies across DtButton, DtBadge, DtBanner, DtNotice, DtToast, DtModal, DtLink, DtCheckbox, DtRadio, DtInput, DtSelectMenu, and related validation message constants.
There was a problem hiding this comment.
|
✔️ Deploy previews ready! |
2 participants
🛠️ Type Of Change
📖 Jira Ticket
DLT-3312
📖 Description
Show resolved CSS values and color swatches alongside raw token prop values in Combinator dropdowns and segmented controls (e.g., "200" → "16px" for spacing, "14px" for icon size, "16px / 1.6" for typography, colored circle for tone/kind/type).
defaultskey in variant files fortokenCategoryannotations across all variantsdisableValuesexclusion type — disables individual prop values based on conditionscontent-width="anchor"Does not annotate composite
sizeprops (Button, Input, etc.) where size maps to multiple CSS properties.💡 Context
Combinator prop controls show abstract token values (100, 200, 300) and color names ("critical", "positive") with no indication of what they resolve to. This adds resolved values and color swatches inline so the mapping is immediately visible.
🔍 For Reviewers
Navigate to component pages:
Dropdown resolved values:
0px,8px,16px,24px,32px...12px,14px,18px...16px,20px,24px,32px...Color swatches:
Segmented tooltip:
1,1.2,1.4,1.6...12px / 1.4,16px / 1.6...Typography + kind reactivity:
bodytoheadline: Size dropdown resolved values update. Sizes 500/600/700 become enabled with values like28px / 1.2,32px / 1.2.Disabled values:
body, sizes 500/600/700 disabled. Switch Kind toheadline→ they become active. Select size 600, kindsbody,label,codedisabled."Font Size / Line Height" tooltip:
📝 Checklist
For all PRs:
🔮 Next Steps
📷 Screenshots / GIFs
Preview Rendered Dimensions
resolved.mp4
Preview color swatch
resolved-swatch.mp4
🔗 Sources
Adds Combinator UI previews showing resolved CSS values (e.g., tokens → pixels) and color swatches. Introduces token category annotations via
defaultskey in variant configs. Implements conditional value disabling withdisableValuesexclusion type. Adds DtText headline-size enforcement logic. Includes token resolution library and exclusion rules.