-
Notifications
You must be signed in to change notification settings - Fork 153
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
Start/stop engine when entering background/foreground #70
Conversation
Hey there @kkiermasz, thank you for the contribution! I took a quick look at the code and I believe that there are two issues that need to be addressed before this change can be merged in.
Because of that I think the likely the right place to make this change is in the caller who instantiates this internal Could I ask you to rework the code with these considerations in mind, so I can merge this in and fix up the spray animation? |
Hey @mergesort, Thanks for the feedback!
#if os(iOS)
import CoreHaptics
internal struct Haptics {
Let me know what you think 😊 |
Would love to hear what you think about this suggestion, would love to get this merged in. 🙂 |
Hey @mergesort, Thanks for the suggestion! Unfortunately, I don't think that would solve the issue. The Given this, I’d like to stick with my solution. I believe the issue might be related to the Let me know your thoughts! |
Hey @kkiermasz, I'm not sure I quite understand what you mean here. I gave the refactor that I had proposed a shot, and looks like you're right! I hadn't realized that the In that case I think you're right to take your approach, and I think we should go with it. I would like to ask you to do one thing though before I can merge it in. Can you take the notification handlers and add them into a private function called This way if someone comes across the code and wonders why this is there (or me in six months when I forget why we added this), they'll be able to understand why this code that otherwise would look out of place exists. Thanks a lot, and I appreciate you being so willing to communicate thoroughly over what is a small code change. |
@mergesort done |
LGTM, thank you for the good work @kkiermasz! |
Hi there,
This PR resolves issue #69.
My original assumption was that
usesCustomHaptics
was created to cover all the scene phase changes. Unfortunately, it does not cover them all. Hard to tell why, as_AppearanceActionModifier
is a hidden API 🤭Steps to reproduce: Check the haptics associated with the
Spray
effect. Then minimize the app and open it again. Before the change, the haptics wouldn't work.With the change, the Spray effect provides a full experience again 🥰
I'm aware of the
@Environment(\.scenePhase)
. However,Haptics
are implemented as a set of static methods, so connectingscenePhases
with it does not make much sense to me, but I'm happy to hear your opinion.Haptics
are shipped to iOS targets only, so usingNotificationCenter
is safe.