shaps80 / SwiftUIBackports

A collection of SwiftUI backports for iOS, macOS, tvOS and watchOS
MIT License
931 stars 59 forks source link

Adds interactiveDismissDisabled backport #16

Closed anivaros closed 1 year ago

anivaros commented 1 year ago

Describe your changes

This adds a func interactiveDismissDisabled(_ isDisabled: Bool = true) backport.

shaps80 commented 1 year ago

Hey, thank you for the submission.

So I actually missed the official implementation for this, didn’t realise we had an official (iOS 15+) modifier so thanks for bring it to my attention.

However I did already include an implementation of this, just with a different name: https://github.com/shaps80/SwiftUIBackports/blob/main/Sources/SwiftUIBackports/Extras/InteractiveDismiss.swift

Can I suggest you update the naming of my original implementation, keeping any docs you’ve added here as they’re def an improvement.

My implementation has a 2nd (unofficial) implementation that enables the user to get a callback when the dismiss attempt occurs. This enables building ‘confirmation’ style UX, so I’d like to keep that.

anivaros commented 1 year ago

Hello! I've updated backport code and now it's using your implementation. Please check this out.

shaps80 commented 1 year ago

Hey thanks! Actually I wasn't clear sorry about that. What I meant was to rename my implementations. So we'd end up with:

interactiveDismissDisabled(_ disabled: Bool = true) -> some View
interactiveDismissDisabled(_ disabled: Bool = true, onAttempt: @escaping () -> Void) -> some View

Note: the closure here shouldn't be optional since passing nil would be the same as using the first modifier.

Also we should move the first one to its own file so it goes inside the Public folder. The onAttempt version should stay in Extras since it's unofficial API.

shaps80 commented 1 year ago

Hope that's clearer. Sorry for confusion. I really appreciate the contribution 👍👍

anivaros commented 1 year ago

But if rename old implementations, people who already using it will get an error. And it will conflict with apple implementation.

we should move the first one to its own file

Both methods depends on private Representable class, that's why I call in backport func your old func

shaps80 commented 1 year ago

Good point I missed that.

So i think we implement my suggestion, then just include the current api (calling these new methods) for backward compat.

These should also be marked as deprecated:

@obsoleted(swift, renamed: "interactiveDismissDisabled(_:)")
func presentation(_ isModal: Bool) -> some View

Also we should update the README to include the new APIs (removing the old one).

I'm on phone atm so code might be wrong haha

anivaros commented 1 year ago

Why do you want keep two separate methods? At first I did exactly as you suggest. But then I've merged two into one because using deprecated method in backport results warning in backport's code.

shaps80 commented 1 year ago

This library should support official API, to simplify migration later on. However I wasn’t previously aware of the official API and so I named it differently.

Implementing both the official and the unofficial methods with the same modifier name, ensures easy/consistent discovery for an API consumer.

I like your contribution, which would bring it inline with the official API. However as you mentioned, we now have users that may be using the existing API. In order for me to remove it in a future version I need users to move away from it. That’s the point of deprecations.

If you’re uncomfortable with this change I can do it myself but I thought since you’ve already started the work, you may want your contribution included 👍🏻

Thanks again.

anivaros commented 1 year ago

No, I asking about presentation(isModal: Bool) and presentation(isModal: Bool = true, _ onAttempt: @escaping () -> Void). Why not just use single method which I made presentation(isModal: Bool = true, _ onAttempt: (() -> Void)? = nil) that covers all cases?

shaps80 commented 1 year ago

No, I asking about presentation(isModal: Bool) and presentation(isModal: Bool = true, _ onAttempt: @escaping () -> Void). Why not just use single method which I made presentation(isModal: Bool = true, _ onAttempt: (() -> Void)? = nil) that covers all cases?

To be honest I used to do this but I often found this problematic.

For example the onAttempt is unofficial and provided as an 'extra' feature that's not part of the official backport.

And in many cases the implementation backing those methods would then be different as well. While that's not the case in this instance, I still prefer (for consistency) to define the methods separately. This also mirrors how  themselves tend to vend API.

IMO it also simplifies documentation, deprecations and code change over time.

shaps80 commented 1 year ago

Hey looks like this wasn't moving so I pulled the changes manually into my code.

I've also decided on some other minor changes based on another PR, so its all going in but from my end sorry about that 👍