Skip to content

feat(SettingsDialog): add close button - #829

Open
netchampfaris wants to merge 1 commit into
mainfrom
feat/settings-dialog-close-button
Open

feat(SettingsDialog): add close button#829
netchampfaris wants to merge 1 commit into
mainfrom
feat/settings-dialog-close-button

Conversation

@netchampfaris

@netchampfaris netchampfaris commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Problem

SettingsDialog renders the base Dialog in bare mode, and in bare mode the base Dialog deliberately skips its own close button. That left the settings dialog with no visible close affordance — users could only dismiss it via Esc or a backdrop click, which isn't discoverable.

Change

Add a floating ghost X button in the dialog's top-right corner. It closes the dialog by setting the v-model, so route-driven consumers (e.g. Gameplan, whose open state is derived from the URL) still react through their own open-state watcher — no consumer changes needed.

  • Made the TabsRoot container relative to anchor the button.
  • Button styling mirrors the standalone close button already used by the base Dialog (ghost variant, lucide-x, text-ink-gray-9).

Verification

Verified in Gameplan against the local demo site: opening Settings shows the X in the top-right (no overlap with panel header content), and clicking it closes the dialog and navigates back to the underlying page.

Docs preview: https://ui.frappe.io/pr-preview/pr-829/

Coverage: 68.81% (+0.01% vs main)

The SettingsDialog renders the base Dialog in bare mode, which suppresses
Dialog's own close button. That left the dialog with no visible close
affordance — only Esc or a backdrop click. Add a floating ghost X button
in the top-right corner that closes the dialog via its v-model, so
route-driven consumers still react through their own open-state watcher.
@greptile-apps

greptile-apps Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

Minimal, well-scoped change that adds a missing UI affordance without altering any logic or API surface.

The button placement, styling, and close mechanism all match the existing base Dialog pattern verbatim, and the relative positioning added to TabsRoot is the correct anchor for the absolute button.

No files require special attention.

Reviews (1): Last reviewed commit: "feat(SettingsDialog): add close button" | Re-trigger Greptile

@barista-for-frappe

Copy link
Copy Markdown

Looks good — adds a floating close X to SettingsDialog, which runs the base Dialog in bare mode (no built-in close button).

  • Mirrors the base Dialog close button exactly (Dialog.vue:87-91): ghost Button + #icon slot + label="Close". #icon is the sanctioned carve-out, and label keeps it accessible. No new props/slots/events — nothing added to the public surface.
  • @click="modelValue = false" sets the model, so route-driven consumers still react through their own open-state watcher.

Nit (not blocking):

  • SettingsDialog.cy.ts covers Esc/v-model close (:117) but not the new button. A one-line spec (click the X → update:modelValue false) would lock it in.

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.

1 participant