feat(tabs, segmented-control) DLT-3292 DLT-3293 DLT-3294 traveling indicator#1192
feat(tabs, segmented-control) DLT-3292 DLT-3293 DLT-3294 traveling indicator#1192Francis Rupert (francisrupert) wants to merge 15 commits intonextfrom
Conversation
Francis Rupert (francisrupert)
added
the
no-visual-test
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a traveling-selection indicator: new transition token and utility classes, CSS hooks/pseudo-elements for FLIP-style animations, a Web Animations API helper and Vue composable, component integrations for tabs/segmented controls (prop + flow changes), and documentation/demo updates (including demo HTML adjustments and an extensive stress-test page). Changes
Sequence DiagramsequenceDiagram
participant User
participant TabGroup
participant Composable as useIndicatorAnimation
participant Utils as indicatorAnimation.js
participant WebAPI as Web Animations API
User->>TabGroup: select tab (click/keyboard)
TabGroup->>TabGroup: capture previous selection rect/styles
TabGroup->>Composable: animate({ oldRect, newEl, orientation, ... })
Composable->>Utils: animateIndicator(state, options)
Utils->>Utils: compute config, check reduced-motion, cancel prior animations
Utils->>WebAPI: start indicator animation (pseudoElement, duration, easing)
Utils->>WebAPI: start hide-native animation (hideProps)
WebAPI-->>Utils: animations finish
Utils->>Utils: cleanup (cancel hide animation, clear handlers)
Utils-->>Composable: resolved
Composable-->>TabGroup: animation complete
TabGroup-->>User: visual update rendered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dialtone-vue/components/tab/tab_group.vue (1)
407-486:⚠️ Potential issue | 🟠 MajorAdd tests for the new indicator-selection flow before merging.
This adds several non-trivial branches (
showIndicatorTransition,$nextTickhandoff, auto-mode cancellation, and outlined/muted animation paths), but there’s no coverage in the diff for them. At minimum I’d cover the no-animation fallback, auto activation, and the::before/::afterbranches. 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/dialtone-vue/components/tab/tab_group.vue` around lines 407 - 486, The PR adds non-trivial indicator-selection logic but lacks tests; add unit tests that exercise transitionIndicator and selection flow: cover the no-animation fallback path when showIndicatorTransition is false or indicator.canAnimate() is false (assert provideObj.selected changes and animate is not called), the $nextTick animation handoff (spy on indicator.animate and ensure it's called with oldRect/newEl when oldRect exists), the outlined and muted branches (assert pseudoElement is '::before' and indicatorExtra includes boxShadow or backgroundColor respectively), and the auto activation cancellation via selectFocusOnTab/selectTab/selectTabByIndex (when activationMode === 'auto' ensure indicator.cancel is called, provideObj.selected set, and onChange invoked). Use spies/mocks for indicator.animate and indicator.cancel and simulate tab DOM elements to produce oldRect/newContext conditions.
🧹 Nitpick comments (1)
packages/dialtone-vue/common/composables/useIndicatorAnimation.js (1)
17-50: The animation gate doesn't verify WAAPI support, but likely moot given browser targets.
cacheIndicatorConfig()checkswindow.matchMedia(very widely supported) and CSS vars, but doesn't detect Web Animations API capability, thepseudoElementoption, ortranslate/scaleanimation support. If a caller runs on a browser lacking these features,animate()will fail without graceful fallback.However, since Dialtone targets Vue ≥3.2 (which drops IE11 and very old browsers), and pseudoElement + translate/scale have been standard across all modern browsers since ~2020 (Safari 13.1+), this is a theoretical concern rather than a practical blocker. Consider adding explicit feature detection in
cacheIndicatorConfig()only if supporting older Safari versions or non-evergreen browsers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/common/composables/useIndicatorAnimation.js` around lines 17 - 50, The current animation gate in useIndicatorAnimation (animate, canAnimate) relies on config from cacheIndicatorConfig but does not detect Web Animations API or specific feature support (pseudo-element animation and translate/scale transforms), so animateIndicator may throw on non-evergreen browsers; update cacheIndicatorConfig to perform explicit feature detection for WAAPI (e.g., typeof Element.prototype.animate === 'function' or Animation in window), detect pseudo-element animation capability and transform (translate/scale) support, store the result as a flag on the returned config (e.g., config.supportsWAAPI or config.canAnimateTransforms), and then have useIndicatorAnimation’s animate and canAnimate check that flag in addition to prefersReducedMotion before calling animateIndicator or returning true so callers gracefully skip animations when features are unavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dialtone-documentation/docs/utilities/effects/transition.md`:
- Around line 80-82: The v-for array for t when i === 'ttf' incorrectly uses
'quint', producing a non-existent class like .d-ttf-quint; change the array
entry 'quint' to 'out-quint' so the template that renders .d-{{ i }}{{ t ?
`-${t}` : '' }} and the timing variable var(--ttf-{{ t || 'ease' }}) will
produce the correct .d-ttf-out-quint and corresponding --ttf-out-quint variable.
In `@packages/dialtone-css/lib/build/less/utilities/effects.less`:
- Around line 192-195: Remove the old selector .ttf-out-quint and keep the new
.d-ttf-out-quint in effects.less (ensure the rule .d-ttf-out-quint {
transition-timing-function: var(--ttf-out-quint) !important; } remains), run a
repository-wide search for any remaining references to .ttf-out-quint or
.ttf-out and update them to .d-ttf-out-quint where appropriate, and add a note
to the release notes/migration guide documenting the breaking rename so external
consumers can update their class usage.
---
Outside diff comments:
In `@packages/dialtone-vue/components/tab/tab_group.vue`:
- Around line 407-486: The PR adds non-trivial indicator-selection logic but
lacks tests; add unit tests that exercise transitionIndicator and selection
flow: cover the no-animation fallback path when showIndicatorTransition is false
or indicator.canAnimate() is false (assert provideObj.selected changes and
animate is not called), the $nextTick animation handoff (spy on
indicator.animate and ensure it's called with oldRect/newEl when oldRect
exists), the outlined and muted branches (assert pseudoElement is '::before' and
indicatorExtra includes boxShadow or backgroundColor respectively), and the auto
activation cancellation via selectFocusOnTab/selectTab/selectTabByIndex (when
activationMode === 'auto' ensure indicator.cancel is called, provideObj.selected
set, and onChange invoked). Use spies/mocks for indicator.animate and
indicator.cancel and simulate tab DOM elements to produce oldRect/newContext
conditions.
---
Nitpick comments:
In `@packages/dialtone-vue/common/composables/useIndicatorAnimation.js`:
- Around line 17-50: The current animation gate in useIndicatorAnimation
(animate, canAnimate) relies on config from cacheIndicatorConfig but does not
detect Web Animations API or specific feature support (pseudo-element animation
and translate/scale transforms), so animateIndicator may throw on non-evergreen
browsers; update cacheIndicatorConfig to perform explicit feature detection for
WAAPI (e.g., typeof Element.prototype.animate === 'function' or Animation in
window), detect pseudo-element animation capability and transform
(translate/scale) support, store the result as a flag on the returned config
(e.g., config.supportsWAAPI or config.canAnimateTransforms), and then have
useIndicatorAnimation’s animate and canAnimate check that flag in addition to
prefersReducedMotion before calling animateIndicator or returning true so
callers gracefully skip animations when features are unavailable.
🪄 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
Run ID: 62318f2b-2a6d-4cf1-9cbd-edcaee296f09
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/dialtone-documentation/docs/components/tabs.mdapps/dialtone-documentation/docs/scratch.mdapps/dialtone-documentation/docs/utilities/effects/transition.mdpackages/dialtone-css/lib/build/less/components/segmented-control.lesspackages/dialtone-css/lib/build/less/components/tabs.lesspackages/dialtone-css/lib/build/less/utilities/effects.lesspackages/dialtone-css/lib/build/less/variables/visual-styles.lesspackages/dialtone-vue/common/composables/useIndicatorAnimation.jspackages/dialtone-vue/common/utils/index.jspackages/dialtone-vue/common/utils/indicatorAnimation.jspackages/dialtone-vue/components/segmented_control/segmented_control.vuepackages/dialtone-vue/components/tab/tab_group.vue
apps/dialtone-documentation/docs/utilities/effects/transition.md
Outdated
Show resolved
Hide resolved
Francis Rupert (francisrupert)
left a comment
Francis Rupert (francisrupert)
left a comment
There was a problem hiding this comment.
Addressed both CodeRabbit findings:
-
'quint'→'out-quint'in transition.md — Fixed. The table now correctly renders.d-ttf-out-quintandvar(--ttf-out-quint). -
.ttf-out-quintrename — Acknowledged. No internal usages of the old unprefixed class. Will note in release notes.
Thanks CodeRabbit!
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
https://github.com/dialpad/dialtone/blob/c82b2e449504d8f5206fa8e6805cd332d2b2df6d/packages/dialtone-vue/common/utils/indicatorAnimation.js#L122-L124
The delta is calculated on only one axis based on orientation, but horizontal tablists can wrap onto multiple rows. When moving selection between wrapped rows, vertical displacement is ignored, so the indicator animates from the wrong origin or skips entirely when the x-centers match (delta === 0). This breaks the traveling-indicator behavior for multi-row tab groups.
ℹ️ 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".
packages/dialtone-vue/components/segmented_control/segmented_control.vue
Outdated
Show resolved
Hide resolved
|
Addressing the CodeRabbit review summary: Inline comments (both resolved):
Outside diff comment (unit tests for Not adding. The animation code gracefully degrades in jsdom ( Nitpick (WAAPI feature detection): Already handled. Both components guard with |
|
Re: Codex review — multi-row tab wrapping Good catch. Fixed — we now detect when old and new elements are on different rows (cross-axis delta > 1px) and skip the animation entirely (instant switch). We don't want a diagonal transition, and wrapped tabs are an extreme edge case that ideally wouldn't occur in practice. |
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/dialtone-vue/common/utils/indicatorAnimation.js`:
- Around line 53-88: Add unit tests covering animateIndicator's decision and
cleanup paths: mock Element.prototype.animate and verify behavior for (1)
cross-axis skip (when crossAxisDelta > 1) returns without starting animations,
(2) center-align skip (when delta === 0) returns without starting animations,
(3) cancel/restart: when state has existing animations ensure
cancelIndicatorAnimations is invoked and new animations are created on
state.hideNative and state.indicator, and (4) onfinish cleanup: simulate
finishing state.indicator and assert state.hideNative.cancel was called; test
both orientations (vertical/horizontal) and include cases using pseudoElement
and indicatorExtra to ensure those options are passed through. Use
animateIndicator and cancelIndicatorAnimations identifiers to locate the code
and assert expected calls on state.hideNative, state.indicator, and the mocked
Element.animate.
🪄 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
Run ID: 4b82775e-cddd-4ca6-8b5c-e1911ee9a90a
📒 Files selected for processing (1)
packages/dialtone-vue/common/utils/indicatorAnimation.js
|
✔️ Deploy previews ready! |
1 participant
🛠️ Type Of Change
📖 Jira Ticket
--ttf-easetoken📖 Description
Adds a "traveling indicator" animation to DtTabGroup and DtSegmentedControl. When clicking a new tab or segmented control item, the selection indicator (underline, border, or background) visually slides from the old selection to the new one instead of instantly switching.
What it does:
Element.animateon pseudo-elements) — no markup changesuseIndicatorAnimationcomposable +indicatorAnimation.jsutilityshowIndicatorTransitionprop on both components (defaulttrue) for consumer opt-out--tab-indicator-duration,--segmented-indicator-duration, etc.)prefers-reduced-motionactivation-mode="auto") snaps instantly — no animationWhat it does not do:
Other changes in this PR:
--ttf-easetoken (cubic-bezier(0.25, 0.1, 0.25, 1)) +.d-ttfutility class.ttf-out-quint→.d-ttf-out-quint(missingd-prefix)🔍 For Reviewers
Test on the deploy preview:
showIndicatorTransition=false, extreme width variance, many tabs, multiple instances.📦 Cross-Package Impact
::before/::afteroverlay rules, new--ttf-easetokentab_group.vue,segmented_control.vue, new composable + utility📝 Checklist
For all PRs:
For all Vue changes:
For all CSS changes:
📷 Screenshots / GIFs / Screencasts
Screen.Recording.2026-04-11.at.9.19.39.PM.mov
Screen.Recording.2026-04-11.at.10.05.10.PM.mov
Adds a "traveling indicator" animation for DtTabGroup and DtSegmentedControl using FLIP + Web Animations API: new CSS vars and pseudo-element overlays, shared indicatorAnimation util and useIndicatorAnimation composable, showIndicatorTransition prop (default true), respects prefers-reduced-motion and keyboard auto-activation.