-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Polish sidebar extensions and titlebar chrome #5017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
d25e768
be98620
71f72e3
14c5900
94d8e66
85cba8b
b233580
e87c66a
eeecf66
fddb559
bc03c07
a44945b
019b269
30f248c
2562d72
6a68659
b0af2f9
09648c1
99d57bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,8 +15,8 @@ enum SessionSnapshotSchema { | |
| enum SessionPersistencePolicy { | ||
| static let sidebarMinimumWidthKey = "sidebarMinimumWidth" | ||
| static let defaultSidebarWidth: Double = 220 | ||
| static let defaultMinimumSidebarWidth: Double = 190 | ||
| static let minimumSidebarWidth: Double = 190 | ||
| static let defaultMinimumSidebarWidth: Double = 171 | ||
| static let minimumSidebarWidth: Double = 171 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minimum width constant does not match the stated product target. Lines 18–19 set the minimum to Proposed fix- static let defaultMinimumSidebarWidth: Double = 171
- static let minimumSidebarWidth: Double = 171
+ static let defaultMinimumSidebarWidth: Double = 180
+ static let minimumSidebarWidth: Double = 180🤖 Prompt for AI Agents |
||
| static let sidebarMinimumWidthRange: ClosedRange<Double> = 120...260 | ||
| static let maximumSidebarWidth: Double = 600 | ||
| static let minimumWindowWidth: Double = 300 | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -453,8 +453,8 @@ enum TitlebarControlsLayoutMetrics { | |||||||||
| static let hintRightSafetyShift: CGFloat = 6 | ||||||||||
| static let hintTrailingBaseInset: CGFloat = 4 | ||||||||||
| static let extraButtonCount = 0 | ||||||||||
| static let trafficLightGap: CGFloat = 2 | ||||||||||
| static let trafficLightClusterWidth: CGFloat = 48 | ||||||||||
| static let trafficLightGap: CGFloat = 0 | ||||||||||
| static let trafficLightClusterWidth: CGFloat = 45 | ||||||||||
|
|
||||||||||
| static func hintTrailingInset(titlebarShortcutHintXOffset: Double = ShortcutHintDebugSettings.defaultTitlebarHintX) -> CGFloat { | ||||||||||
| max(0, ShortcutHintDebugSettings.clamped(titlebarShortcutHintXOffset)) | ||||||||||
|
|
@@ -468,15 +468,19 @@ enum TitlebarControlsLayoutMetrics { | |||||||||
| return (buttonCount * config.buttonSize) + (gapCount * config.spacing) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| static func visibleContentWidth(config: TitlebarControlsStyleConfig) -> CGFloat { | ||||||||||
| outerLeadingPadding | ||||||||||
| + config.groupPadding.leading | ||||||||||
| + buttonRowWidth(config: config) | ||||||||||
| + config.groupPadding.trailing | ||||||||||
| } | ||||||||||
|
|
||||||||||
| static func contentSize( | ||||||||||
| config: TitlebarControlsStyleConfig, | ||||||||||
| titlebarShortcutHintXOffset: Double = ShortcutHintDebugSettings.defaultTitlebarHintX | ||||||||||
| ) -> NSSize { | ||||||||||
| NSSize( | ||||||||||
| width: outerLeadingPadding | ||||||||||
| + config.groupPadding.leading | ||||||||||
| + buttonRowWidth(config: config) | ||||||||||
| + config.groupPadding.trailing | ||||||||||
| width: visibleContentWidth(config: config) | ||||||||||
| + hintTrailingInset(titlebarShortcutHintXOffset: titlebarShortcutHintXOffset), | ||||||||||
| height: max( | ||||||||||
| WindowChromeMetrics.appTitlebarHeight, | ||||||||||
|
|
@@ -514,7 +518,7 @@ enum TitlebarControlsLayoutMetrics { | |||||||||
| static func minimumSidebarWidth(config: TitlebarControlsStyleConfig) -> CGFloat { | ||||||||||
| trafficLightClusterWidth | ||||||||||
| + trafficLightGap | ||||||||||
| + contentSize(config: config).width | ||||||||||
| + visibleContentWidth(config: config) | ||||||||||
| + sidebarTrailingPadding | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -2010,6 +2014,7 @@ final class TitlebarControlsAccessoryViewController: NSTitlebarAccessoryViewCont | |||||||||
| let styleRawValue = UserDefaults.standard.integer(forKey: "titlebarControlsStyle") | ||||||||||
| let style = TitlebarControlsStyle(rawValue: styleRawValue) ?? .classic | ||||||||||
| let contentSize = TitlebarControlsLayoutMetrics.contentSize(config: style.config) | ||||||||||
| let visibleContentWidth = TitlebarControlsLayoutMetrics.visibleContentWidth(config: style.config) | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||
| if intrinsicSizeNeedsRefresh { | ||||||||||
| hostingView.invalidateIntrinsicContentSize() | ||||||||||
| intrinsicSizeNeedsRefresh = false | ||||||||||
|
|
@@ -2038,7 +2043,7 @@ final class TitlebarControlsAccessoryViewController: NSTitlebarAccessoryViewCont | |||||||||
| let accessoryOriginXInWindow = view.convert(view.bounds, to: nil).minX | ||||||||||
| let sidebarTrailingEdgeInAccessory = max(0, sidebarTrailingEdge - accessoryOriginXInWindow) | ||||||||||
| let xOffset = TitlebarControlsLayoutMetrics.leadingOffset( | ||||||||||
| contentWidth: contentSize.width, | ||||||||||
| contentWidth: visibleContentWidth, | ||||||||||
| trafficLightFrame: trafficLightFrame, | ||||||||||
| debugSnapshot: debugSnapshot, | ||||||||||
| sidebarTrailingEdge: sidebarTrailingEdgeInAccessory | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,19 +16,19 @@ final class SidebarWidthPolicyTests: XCTestCase { | |
|
|
||
| XCTAssertEqual( | ||
| SessionPersistencePolicy.defaultMinimumSidebarWidth, | ||
| 190, | ||
| 171, | ||
| accuracy: 0.001 | ||
| ) | ||
| XCTAssertEqual( | ||
| SessionPersistencePolicy.resolvedMinimumSidebarWidth(defaults: defaults), | ||
| 190, | ||
| 171, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift Convert touched XCTest tests to Swift Testing per repo policy. This file is modified but still uses As per coding guidelines: “When touching an existing XCTest test, convert in place: Also applies to: 31-31 🤖 Prompt for AI Agents |
||
| accuracy: 0.001 | ||
| ) | ||
| } | ||
|
|
||
| func testContentViewClampKeepsMinimumSidebarWidth() { | ||
| XCTAssertEqual( | ||
| ContentView.clampedSidebarWidth(184, maximumWidth: 600), | ||
| ContentView.clampedSidebarWidth(164, maximumWidth: 600), | ||
| CGFloat(SessionPersistencePolicy.minimumSidebarWidth), | ||
| accuracy: 0.001 | ||
| ) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.