Skip to content

Remove duplicate close button from OnboardingWalkthrough dialog#988

Merged
hotlong merged 2 commits into
mainfrom
copilot/fix-onboarding-dialog-close-buttons
Mar 3, 2026
Merged

Remove duplicate close button from OnboardingWalkthrough dialog#988
hotlong merged 2 commits into
mainfrom
copilot/fix-onboarding-dialog-close-buttons

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 3, 2026

OnboardingWalkthrough manually renders a close <Button> in the DialogHeader, but DialogContent already includes a DialogPrimitive.Close button at the same position — producing two overlapping × buttons.

  • Removed the manual <Button><X /></Button> and its wrapping flex div from the header
  • Removed unused X import from lucide-react

Dismissal continues to work via DialogContent's built-in close button, which triggers Dialog's onOpenChangedismiss(). ESC and overlay click are unaffected.

Original prompt

This section details on the original issue you should resolve

<issue_title>Onboarding dialog renders two overlapping close (×) buttons</issue_title>
<issue_description>## Problem

The Onboarding dialog displays two overlapping close (×) buttons in the upper-right corner (see screenshot below):

Onboarding dialog with two close buttons

Cause

  • The OnboardingWalkthrough component manually renders its own close button in the header:
    • <Button variant="ghost" size="icon" ...><X ... /></Button>
  • The DialogContent component (from @object-ui/components) already includes a built-in close button positioned absolutely at the top-right corner.
  • These two buttons appear at the same location, resulting in a duplicated UI control and confusion.

Example (from code):

// In OnboardingWalkthrough.tsx
<DialogHeader>
  <DialogTitle>...</DialogTitle>
  <Button ...><X ... /></Button> // Manually added close button
</DialogHeader>

// In DialogContent (auto-generated via Radix)
<DialogPrimitive.Close ...>
  <X ... />
</DialogPrimitive.Close>

Solution

  • Remove the manually rendered close button from the OnboardingWalkthrough's DialogHeader, as DialogContent already provides an accessible close control.
  • Verify that dismissing the dialog still works via the remaining button and that keyboard/escape functionality behaves correctly via Dialog's onOpenChange.

References

Screenshot

Screenshot showing duplicate close buttons

Severity

Minor – does not block usage but affects UX polish.


Test after fix:

  • Only one close button visible
  • Dialog dismissal works as expected (button, ESC, clicking outside)
  • WCAG accessibility for close button touch targets preserved

Related file: apps/console/src/components/OnboardingWalkthrough.tsx


After completing this task: run tests, update related issues, and update ROADMAP.md. If a change requires work in another ObjectStack repo, create a cross-repo issue in the target repo.</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 3, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
objectui Canceled Canceled Mar 3, 2026 5:27am
objectui-demo Ready Ready Preview, Comment Mar 3, 2026 5:27am
objectui-storybook Ready Ready Preview, Comment Mar 3, 2026 5:27am

Request Review

The DialogContent component already provides a built-in accessible
close button via Radix DialogPrimitive.Close. Removed the manually
rendered close button and its wrapping div from the DialogHeader to
eliminate the two overlapping × buttons in the upper-right corner.

Also removed the unused X import from lucide-react.

Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix overlapping close buttons in onboarding dialog Remove duplicate close button from OnboardingWalkthrough dialog Mar 3, 2026
@hotlong hotlong marked this pull request as ready for review March 3, 2026 05:35
Copilot AI review requested due to automatic review settings March 3, 2026 05:35
@hotlong hotlong merged commit bc1303c into main Mar 3, 2026
5 checks passed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes a redundant close button from the OnboardingWalkthrough dialog header so the dialog relies on the shared DialogContent built-in close control (avoiding two overlapping × buttons).

Changes:

  • Removed the manually-rendered header close button and its wrapper layout.
  • Removed the unused X import from lucide-react.

Comment on lines +104 to +107
<DialogTitle className="flex items-center gap-2">
<Rocket className="h-5 w-5 text-primary" />
Welcome to ObjectUI
</DialogTitle>
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change removes the custom close button, but there isn't a regression test ensuring the dialog renders only a single close control and that clicking the remaining built-in close button still calls dismiss() (persisting to localStorage). Consider adding a test that asserts a single Close-labeled button and verifies dismissal via that button.

Copilot uses AI. Check for mistakes.
@@ -102,16 +101,10 @@ export function OnboardingWalkthrough() {
<Dialog open={open} onOpenChange={(v: any) => { if (!v) dismiss(); }}>
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onOpenChange is typed as (v: any) even though Radix Dialog passes a boolean. Now that dismissal relies on this callback, it would be better to type this parameter as boolean (and optionally update state from v) to keep type-safety and avoid masking mistakes.

Suggested change
<Dialog open={open} onOpenChange={(v: any) => { if (!v) dismiss(); }}>
<Dialog open={open} onOpenChange={(v: boolean) => { if (!v) dismiss(); }}>

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Onboarding dialog renders two overlapping close (×) buttons

3 participants