Skip to content

feat(mobile): show full setting descriptions - #187

Closed
Lint111 wants to merge 1 commit into
Ark0N:masterfrom
Lint111:agent/split-mobile-setting-descriptions
Closed

feat(mobile): show full setting descriptions#187
Lint111 wants to merge 1 commit into
Ark0N:masterfrom
Lint111:agent/split-mobile-setting-descriptions

Conversation

@Lint111

Copy link
Copy Markdown
Contributor

Summary

Let phone users open the full description of an App Settings option without toggling or editing that option.

Compact mobile setting tiles truncate long explanations. This adds an accessible bottom-sheet description layer while leaving the existing controls and desktop title tooltips unchanged.

Behavior

  • Marks titled settings as description-capable on phone layouts.
  • Opens the full title text by tapping the setting's text area.
  • Keeps switches, inputs, selects, links, and action buttons on their existing direct-interaction path.
  • Supports keyboard activation with Enter or Space.
  • Closes from the backdrop, close button, or Escape.
  • Temporarily makes the parent settings content inert and hands focus trapping to the description dialog.
  • Restores the parent settings focus trap after closing.
  • Removes mobile-only roles and trigger styling on wider layouts.

Accessibility

  • role="dialog" and aria-modal="true" on the description panel.
  • Labelled/described relationships for title and body text.
  • Trigger metadata via role="button", tabindex, aria-haspopup, and aria-controls.
  • 44px close target and visible focus treatment.
  • Long labels and descriptions wrap within the phone viewport.

Scope Correction

The original test referenced the terminal-controls setting from another feature branch. This split uses the existing upstream Wheel Scrolls Local History setting instead, so the PR targets master independently.

Verification

  • Focused Playwright regression:
    • npx vitest run --config test/mobile/vitest.config.ts test/mobile/settings.test.ts -t "opens the full option description"
    • 1 passed, 19 skipped
  • Full mobile settings file:
    • 18 passed, 2 failed
    • Both failures reproduce unchanged on origin/master:
      • mobile defaults: all panels hidden, subagent tracking OFF, ralph OFF
      • settings survive page reload
  • npx prettier --check test/mobile/settings.test.ts
  • npm run check:frontend-syntax
  • npm run check:public-assets
  • npm run build

Non-Goals

  • Rewriting or shortening existing setting copy
  • Changing desktop settings layout or tooltip behavior
  • Changing any setting value, persistence policy, or default

@Lint111
Lint111 marked this pull request as ready for review August 2, 2026 05:43
CopilotAI review requested due to automatic review settings August 2, 2026 05:43

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a mobile-only “full description” bottom-sheet to App Settings so phone users can read long setting explanations without accidentally toggling or editing the setting, while preserving existing desktop tooltip behavior.

Changes:

  • Adds a settings description layer (dialog) that opens by tapping a setting’s text region on phone layouts, including focus-trap/inert handling and Escape/backdrop/close-button dismissal.
  • Styles the bottom-sheet overlay and adds a visual affordance for description-capable settings on mobile.
  • Adds a Playwright mobile regression test to ensure opening the description does not toggle the associated setting and that dismissal works.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

FileDescription
test/mobile/settings.test.tsAdds a regression test covering the new “open full description” interaction and ensuring the switch state doesn’t change.
src/web/public/settings-ui.jsImplements the mobile description layer creation, trigger wiring, and focus/inert behavior within the App Settings modal.
src/web/public/mobile.cssAdds mobile-only styling for the description trigger affordance and the bottom-sheet dialog/backdrop.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +655 to +657
if (parentTrap && parentTrap === this.activeFocusTrap) {
parentTrap.element.addEventListener('keydown', parentTrap.boundHandleKeydown);
}
Comment on lines +2092 to +2094
max-width: 420px;
max-height: min(60dvh, 420px);
overflow-y: auto;
@Ark0N

Ark0N commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Hi Lior, closing this one as part of a cleanup of the seventeen PRs currently open from you. The full explanation is in #173, and I would rather you read that one than this note, because it is the honest version and it is not a dismissal of your work.

The short form: sixteen PRs opened in a single day, roughly 42,000 added lines in total, landing mostly on the same few files (nine touch app.js, eight touch terminal-ui.js), with five already conflicting against master and none carrying CI results. I cannot review that at the standard this codebase needs, and merging it at a lower standard would be worse than not merging it.

Going forward, please keep no more than three open PRs at a time, each one a single behavior change that stands on its own, with a test that fails on master and passes with the fix. #214 and #215, merged today, are good models.

If this particular change fixes something that genuinely annoys you in daily use, it is a good candidate to be the first one you reopen on its own, rebased on current master. I will review it properly.

Thanks for the effort you put in, and sorry to close it this way.

@Ark0NArk0N closed this Aug 5, 2026
Sign up for freeto 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.

3 participants

@Lint111@Ark0N