Skip to content
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

Add navigationTitle (Binding) support #366

Merged
merged 4 commits into from
Feb 14, 2025

Conversation

andy-vanwagoner-jn
Copy link
Contributor

While navigationTitle(String | Text | LocalizedStringKey) is a bit difficult to pin down for inspection, we can get the binding from navigationTitle(Binding<String>) fairly simply. This PR adds navigationTitle() -> String, and setNavigationTitle(String) methods.

I would hope the same navigationTitle() -> String VI method eventually is extended to handle the SwiftUI navigationTitle(String | Text | LocalizedStringKey) cases too.

@andy-vanwagoner-jn andy-vanwagoner-jn marked this pull request as ready for review February 7, 2025 21:54
Comment on lines 51 to 54
@available(iOS 16.0, macOS 13.0, tvOS 16.0, watchOS 9.0, *)
func setNavigationTitle(_ value: String) throws {
try navigationTitleBinding().wrappedValue = value
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm hesitant to add the setter, as this is a side effect naturally not available in the UI. For example, the library exposes setMapRect on Map view, which is a side effect the user can cause from the UI (manually adjusting the map), so I can see a use case when this is used in tests. With the navigation title, by keeping the setter unavailable, the developer would be encouraged to change it programmatically from view's state, which would contribute to their app correctness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you use a binding to set the navigation title, it can be updated from the UI. This is the functionality we are hoping to test in our app.

Screenshot 2025-02-10 at 9 59 08 AM Screenshot 2025-02-10 at 9 59 17 AM

Copy link
Owner

Choose a reason for hiding this comment

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

In this case yeah, it's the user causing the state change that you otherwise cannot imitate. I would also do the same as for the getter, that is rethrow a meaningful error about API limitation


@available(iOS 16.0, macOS 13.0, tvOS 16.0, watchOS 9.0, *)
func navigationTitle() throws -> String {
return try navigationTitleBinding().wrappedValue
Copy link
Owner

Choose a reason for hiding this comment

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

I would recommend when navigationTitleBinding throws here (when the navigationTitle is set up in any other way but Binding), we capture the error and rethrow a InspectionError.notSupported and explain in the error that only the variant with Binding is supported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good idea. I'll try to implement that.

Copy link
Owner

@nalexn nalexn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! A couple of minor tweaks requested

@andy-vanwagoner-jn
Copy link
Contributor Author

@nalexn Let me know if that's acceptable. The system does provide UI automatically for editing a bound navigation title, and I updated the error to indicate only the binding overload is currently supported.

@nalexn nalexn merged commit bb1b4e6 into nalexn:0.10.2 Feb 14, 2025
@andy-vanwagoner-jn andy-vanwagoner-jn deleted the navigationTitleBinding branch February 14, 2025 16:17
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.

2 participants