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

Bookmarks - Add binding selection init parameter #246

Closed
dfeinzimer opened this issue Jan 11, 2023 · 6 comments · Fixed by #549
Closed

Bookmarks - Add binding selection init parameter #246

dfeinzimer opened this issue Jan 11, 2023 · 6 comments · Fixed by #549
Assignees
Labels
enhancement New feature or request

Comments

@dfeinzimer
Copy link
Collaborator

dfeinzimer commented Jan 11, 2023

Update:
See discussion below


Original:
As an alternate to using a viewpoint, the bookmarks component can potentially take advantage of the setBookmark(_:) method on a map or scene view proxy.

@dfeinzimer dfeinzimer added the enhancement New feature or request label Jan 11, 2023
@philium
Copy link
Contributor

philium commented Jan 11, 2023

Alternatively, it could be simplified by taking a binding to the selected bookmark as an initializer parameter, giving the caller flexibility to do whatever they want when the selected bookmark is updated (onChange(of selectedBookmark) { … }). I think that would be better than taking a binding to a viewpoint.

@dfeinzimer
Copy link
Collaborator Author

Closing; onSelectionChanged(perform:) is provided

@philium
Copy link
Contributor

philium commented Dec 16, 2023

@dfeinzimer Was there any consideration given to taking a binding to the selection? While there is the modifier that you pointed out, taking a binding to the selected bookmark would be simpler and more idiomatic in SwiftUI. Many SwiftUI-provided modifiers take a selection binding instead of providing a separate modifier to inform when the selection has changed.

@dfeinzimer
Copy link
Collaborator Author

Yes - I still think we should do it and I thought we had an issue for it but I'm not seeing it so I'll reopen and re-characterize this issue instead.

@dfeinzimer dfeinzimer reopened this Dec 16, 2023
@dfeinzimer dfeinzimer changed the title Should the Bookmarks component support using setBookmark(_:)? Bookmarks - Add binding selection init parameter Dec 16, 2023
@dfeinzimer
Copy link
Collaborator Author

Some additional API considerations:

The current initializers are:

init(isPresented: Binding<Bool>, bookmarks: [Bookmark], viewpoint: Binding<Viewpoint?>?)
&
init(isPresented: Binding<Bool>, bookmarks: [Bookmark], viewpoint: Binding<Viewpoint?>?)

Would we want to add 2 new initializers:
init(isPresented: Binding<Bool>, bookmarks: [Bookmark], selection: Binding<Bookmark?>?)
&
init(isPresented: Binding<Bool>, geoModel: GeoModel, selection: Binding<Bookmark?>?)

brining us to a total of 4? 4 seems like it would be a lot to me.

@philium
Copy link
Contributor

philium commented Dec 18, 2023

The selection parameter should be non-optional because those initializers should only be used with a selection binding. In my opinion, having four initializers isn't a big deal. Several SwiftUI views have much more than four initializers.

@dfeinzimer dfeinzimer linked a pull request Dec 19, 2023 that will close this issue
@dfeinzimer dfeinzimer self-assigned this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants