Conversation
🦋 Changeset detectedLatest commit: 7557998 The changes in this PR will be included in the next version bump. This PR includes changesets to release 83 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code ReviewI've reviewed PR #2929 and found 7 issues that need to be addressed: 1. ❌ Missing Vue/Nuxt ExamplesSeverity: High - CLAUDE.md Violation This PR adds multiple-trigger examples for React (7 examples), Solid (5 examples), and Svelte (5 examples), but completely omits Vue/Nuxt examples. According to the Framework Integration Guide:
Missing Vue/Nuxt examples needed:
Zag.js is framework-agnostic and should maintain feature parity across all supported frameworks. 2. ❌ Inconsistent Event Naming: Dialog vs Other ComponentsSeverity: Medium - CLAUDE.md Violation Dialog uses See: zag/packages/machines/dialog/src/dialog.machine.ts Lines 116 to 119 in f0ddb54 The State Machine Guide establishes that event names should match their context property names. Since all components use Recommendation: Change Dialog's event name from 3. ❌ Incorrect Event Naming in TooltipSeverity: Medium - CLAUDE.md Violation Tooltip uses See: zag/packages/machines/tooltip/src/tooltip.machine.ts Lines 204 to 208 in f0ddb54 The State Machine Guide requires SCREAMING_SNAKE_CASE for event names:
Recommendation: Change 4. ❌ Inconsistent Action Naming: Dialog vs Other ComponentsSeverity: Low - CLAUDE.md Violation Dialog uses Both actions have identical implementations: // Dialog
setActiveTrigger({ context, event }) {
context.set("triggerValue", event.value ?? null)
}
// Other components
setTriggerValue({ context, event }) {
context.set("triggerValue", event.value ?? null)
}Recommendation: Rename Dialog's action from 5. 🐛 Dialog Single-Trigger Toggle BrokenSeverity: Critical - Breaking Bug When a dialog is used with a single trigger (no See: zag/packages/machines/dialog/src/dialog.connect.ts Lines 46 to 49 in f0ddb54 The Problem: const current = value == null ? false : triggerValue === value
const shouldSwitch = open && !current // ❌ Missing value != null guard
send({ type: shouldSwitch ? "ACTIVE_TRIGGER.SET" : "TOGGLE", value })In single-trigger mode, The Fix: const shouldSwitch = open && value != null && !current // ✅ CorrectRecommendation: Add 6. 🐛 Tooltip Single-Trigger Interactions Cause Close-Reopen FlickerSeverity: Critical - Breaking Bug When a tooltip is used with a single trigger, interactions (click/focus/hover) cause a close-then-reopen flicker instead of normal behavior. Affected handlers:
The Problem: const shouldSwitch = open && !current // ❌ Missing value != null guardIn single-trigger mode,
In the Recommendation: Add 7. 🐛 Tooltip Controlled
|
Add support for rendering multiple triggers
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information