Replace remaining action menus with Tuner controls#70
Conversation
|
Copilot review is requested and pending. Local verification remains green: no Menu/contextMenu call sites, git diff --check, simulator build, and 65-unit OffScriptTests pass. |
There was a problem hiding this comment.
Pull request overview
This PR continues OffScript’s “Tuner instrument cluster” UI direction by removing remaining SwiftUI Menu/contextMenu affordances and replacing them with always-visible or inline-expandable Tuner-style controls in core flows (Home hero feedback, compact episode cards, and Queue reordering/removal).
Changes:
- Replaces Queue row long-press
contextMenureorder/remove actions with inline move up/down + remove controls. - Replaces Home hero-card feedback
Menuwith an inline expandable row of feedback keys. - Replaces compact episode-card action
Menuwith an inline expandable row for queue + played/unplayed actions; updates changelog accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| OffScript/QueueView.swift | Adds inline move up/down buttons and removes the queue row contextMenu. |
| OffScript/HomeView.swift | Replaces hero feedback Menu with an inline “expand actions” pattern and helper functions. |
| OffScript/CardComponents.swift | Replaces compact-card Menu with an inline “expand actions” row and shared button styling helper. |
| CHANGELOG.md | Notes the removal of remaining system action chrome in these flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private func tunerInlineAction( | ||
| title: String, | ||
| color: Color, | ||
| disabled: Bool = false, | ||
| action: @escaping () -> Void | ||
| ) -> some View { | ||
| Button(action: action) { | ||
| TunerLabel(text: title, color: color.opacity(disabled ? 0.55 : 1), size: 9) | ||
| .padding(.horizontal, 10) | ||
| .padding(.vertical, 7) | ||
| .frame(minHeight: 34) | ||
| .overlay(Rectangle().stroke(color.opacity(disabled ? 0.35 : 0.7), lineWidth: 1)) | ||
| } | ||
| .disabled(disabled) | ||
| .buttonStyle(.plain) |
There was a problem hiding this comment.
tunerInlineAction uses .frame(minHeight: 34), which yields a touch target smaller than 44pt. Given the file’s own comments about 44pt hit targets, consider bumping this to at least 44 (or otherwise ensuring a 44×44 tappable area via padding/contentShape).
| } | ||
| .padding(.leading, rank == nil ? 64 : 92) | ||
| .transition(.opacity.combined(with: .move(edge: .top))) |
There was a problem hiding this comment.
The inline actions row is aligned using hard-coded leading padding values (64 / 92). This is brittle if the leading content widths change (rank shown/hidden, Dynamic Type, icon sizing). Consider aligning the actions row with the text column using layout primitives (e.g., a leading column with fixed widths, alignment guides, or a Grid) instead of magic numbers.
| .foregroundStyle(color.opacity(disabled ? 0.35 : 1)) | ||
| .frame(width: 24, height: 28) | ||
| .overlay(Rectangle().stroke(Color.offscriptHairline.opacity(disabled ? 0.35 : 1), lineWidth: 1)) | ||
| .frame(width: 36, height: 44) |
There was a problem hiding this comment.
queueIconButton sets the button label frame to width: 36, height: 44, which makes the tap target narrower than the common 44×44 minimum used elsewhere in the UI. Consider making the outer frame at least 44pt wide (and/or adding padding) so the move controls are easier to hit, especially for accessibility.
| .frame(width: 36, height: 44) | |
| .frame(width: 44, height: 44) |
| @State private var showsFeedbackActions = false | ||
|
|
There was a problem hiding this comment.
showsFeedbackActions is @State, so when the hero episode changes (new leadScoredEpisode), the expanded feedback row can remain visible for the next episode. Consider resetting this state when episode.id changes (e.g., .onChange(of: episode.id) { _ in showsFeedbackActions = false }) or giving the view an identity tied to the episode (e.g., .id(episode.id)).
| TunerLabel(text: title, color: color, size: 9) | ||
| .padding(.horizontal, 10) | ||
| .padding(.vertical, 7) | ||
| .frame(minHeight: 34) |
There was a problem hiding this comment.
preferenceKey uses .frame(minHeight: 34), which creates a touch target below the 44pt minimum referenced elsewhere in the codebase (and Apple's HIG). Consider increasing the minimum height (or adding padding/contentShape) so each feedback key reliably meets a 44×44 hit area.
| .frame(minHeight: 34) | |
| .frame(minHeight: 44) | |
| .contentShape(Rectangle()) |
|
Addressed Copilot review feedback in 77be082. Changes made:\n\n- Raised compact-card inline action targets to 44pt with explicit content shapes.\n- Removed brittle leading-padding alignment and aligned compact-card expanded actions through the same fixed leading columns as the row.\n- Raised queue move/remove controls to 44x44.\n- Reset hero feedback expansion when the lead episode changes.\n- Raised hero feedback keys to 44pt with explicit content shapes.\n\nVerification rerun:\n- rg -n 'Menu {|contextMenu' OffScript -> no matches\n- git diff --check\n- xcodebuild build -project OffScript.xcodeproj -scheme OffScript -destination 'platform=iOS Simulator,OS=latest,name=iPhone 17 Pro'\n- xcodebuild test -project OffScript.xcodeproj -scheme OffScript -destination 'platform=iOS Simulator,OS=latest,name=iPhone 17 Pro' -only-testing:OffScriptTests -> 65 tests passed |
Summary
Verification
Notes