Skip to content

feat(settings): add toast feedback when saving settings or panel layout#1269

Closed
Niboshi-Wasabi wants to merge 5 commits intokoala73:mainfrom
Niboshi-Wasabi:feat/settings-save-feedback
Closed

feat(settings): add toast feedback when saving settings or panel layout#1269
Niboshi-Wasabi wants to merge 5 commits intokoala73:mainfrom
Niboshi-Wasabi:feat/settings-save-feedback

Conversation

@Niboshi-Wasabi
Copy link
Copy Markdown
Contributor

Adds visual confirmation (toast) when saving settings so users get clear feedback.

Changes:

  • UnifiedSettings.ts: Added showSaveToast() that shows a short-lived toast using the existing .toast-notification pattern. Called when the user saves panel layout (Panels tab Save button). role="status" for accessibility.
  • UnifiedSettings.ts: Passes onSettingSaved: () => this.showSaveToast() to renderPreferences() so the same toast is shown when any preference is changed in the Settings tab.
  • preferences-content.ts: PreferencesHost now has optional onSettingSaved. All preference change handlers (theme, map provider, map theme, stream quality, globe preset, language, AI toggles, etc.) call it after saving. Import/Export keep their existing in-panel toast only.

Uses existing modals.settingsWindow.saved for the message (no new locale keys).

Ref: #1247 (second of the three approved items: Settings save feedback).

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 8, 2026

@Niboshi-Wasabi is attempting to deploy a commit to the Elie Team on Vercel.

A member of the Team first needs to authorize it.

@koala73
Copy link
Copy Markdown
Owner

koala73 commented Mar 8, 2026

Thanks for the contribution! The toast feedback on save is a solid UX improvement 👍

Feedback

1. Toast duplication — 3 identical implementations

The new showSaveToast() in UnifiedSettings.ts is the 3rd copy of the same global toast logic. We already have two identical ones:

# File Method
1 src/app/event-handlers.ts:958 showToast(msg)
2 src/app/country-intel.ts:844 showToast(msg)
3 New src/components/UnifiedSettings.ts showSaveToast()

All three do the same thing: remove existing .toast-notification → create div → append to document.bodyrequestAnimationFrame → fade after 3s → remove after 300ms. The only difference is your new one adds role="status" (which is actually a good addition the other two are missing).

If you're up for it, it would be great to consolidate these into a single shared utility — something like src/utils/toast.ts:

export function showToast(msg: string): void {
  document.querySelector('.toast-notification')?.remove();
  const el = document.createElement('div');
  el.className = 'toast-notification';
  el.setAttribute('role', 'status');
  el.textContent = msg;
  document.body.appendChild(el);
  requestAnimationFrame(() => el.classList.add('visible'));
  setTimeout(() => {
    el.classList.remove('visible');
    setTimeout(() => el.remove(), 300);
  }, 3000);
}

Then all three call sites just import { showToast } from '@/utils/toast' and call showToast(msg). This also gives us the role="status" fix everywhere for free.

Note: the 4th toast in preferences-content.ts (showToast(container, msg, success)) is different — it's an inline, scoped-to-container toast with success/error styling and a "Reload now" link. That one stays as-is.

2. Self-review doc

docs/SELF_REVIEW_SETTINGS_SAVE_FEEDBACK.md looks like a development process artifact (self-review checklist in Japanese). Should probably be removed from the PR — it's not project documentation.

3. Minor: onSettingSaved?.() call pattern

The 11 separate host.onSettingSaved?.() calls scattered across individual if branches could be simplified to a single call at the end of the handler (with early returns for import/export). Not blocking, just a cleanup suggestion if you're already touching the file.

Let us know if you'd like help with any of these changes!

…ved, remove self-review doc

- Add src/utils/toast.ts with showToast(msg), role=status; use from UnifiedSettings, country-intel; remove duplicate from event-handlers
- preferences-content: single host.onSettingSaved?.() at end of change handler (early return for import)
- Remove docs/SELF_REVIEW_SETTINGS_SAVE_FEEDBACK.md per maintainer feedback

Made-with: Cursor
@Niboshi-Wasabi
Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed all three points:

  1. Toast consolidation — Added src/utils/toast.ts with showToast(msg) (including role="status"). UnifiedSettings and country-intel now import and use it; removed the duplicate method from event-handlers. The inline toast in preferences-content (import/export) is unchanged.

  2. Self-review doc — Removed docs/SELF_REVIEW_SETTINGS_SAVE_FEEDBACK.md from the PR.

  3. onSettingSaved — Refactored the change handler to a single host.onSettingSaved?.() at the end, with early return only for import; preference branches use else-if and fall through to the one call.

Pushed as a follow-up commit on this branch.

@koala73
Copy link
Copy Markdown
Owner

koala73 commented Mar 12, 2026

Thanks for the follow-up commit addressing all the earlier feedback, the toast consolidation into src/utils/toast.ts is clean work!

One thing we noticed: since the original review, we added inline "Saved" feedback to the Panels tab (the panelsJustSaved flag + #usPanelsStatus element in UnifiedSettings.ts, lines 385-406). That means the Panels tab Save button now shows both:

  1. The existing inline "Saved" text next to the button (already working on main)
  2. The new body-level toast from this PR

That's double feedback for the same action on the Panels tab. The toast is still valuable for the Preferences tab (theme, map provider, language, etc.) where there's currently no save confirmation at all.

Could you remove the showToast() call from savePanelChanges() in UnifiedSettings.ts (line 386 in the PR diff) and keep it only for the onSettingSaved callback wired into preferences? That way each tab has exactly one feedback mechanism.

Everything else in the PR looks good to merge once that's addressed.

@Niboshi-Wasabi
Copy link
Copy Markdown
Contributor Author

Sorry for the delayed response (took about a week to catch up). Thanks for the suggestion! I removed the global showToast call from savePanelChanges() in UnifiedSettings.ts, so the Panels tab save now shows only the existing inline Saved feedback. The toast feedback remains wired for the Preferences tab via onSettingSaved (theme/map/language/etc.), as intended. I pushed a follow-up commit to this PR.

@lspassos1
Copy link
Copy Markdown
Collaborator

I reopened this work as #2261 on top of current main.

Reason: the original branch has drifted too far from main and is no longer typechecking cleanly (src/services/preferences-content.ts is broken in the current PR branch). Rather than trying to repair that stale branch, #2261 reapplies the same settings save feedback change set cleanly against the current codebase:

  • shared global toast utility
  • save feedback for Preferences changes
  • no duplicate toast on the Panels tab
  • duplicate toast implementations removed from existing call sites
  • focused guardrail test added

Local validation for #2261:

  • npx tsx --test tests/settings-save-feedback.test.mjs
  • npm run typecheck
  • npx biome lint src/utils/toast.ts src/components/UnifiedSettings.ts src/services/preferences-content.ts src/app/event-handlers.ts src/app/country-intel.ts tests/settings-save-feedback.test.mjs

@koala73 koala73 added High Value Meaningful contribution to the project Not Ready to Merge PR has conflicts, failing checks, or needs work labels Mar 26, 2026
@koala73
Copy link
Copy Markdown
Owner

koala73 commented Mar 26, 2026

Superseded by #2261 which implements the same save feedback with a cleaner approach.

@koala73 koala73 closed this Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

High Value Meaningful contribution to the project Not Ready to Merge PR has conflicts, failing checks, or needs work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants