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

Reset tintAdjustmentMode for presentationUndimmed #22

Merged
merged 1 commit into from Aug 31, 2022

Conversation

schiewe
Copy link
Contributor

@schiewe schiewe commented Aug 31, 2022

Have you read the Contributing Guidelines?

Yes! 🎉

Issue #

Describe your changes

As discussed here: https://danielsaidi.com/blog/2022/06/21/undimmed-presentation-detents-in-swiftui
tintAdjustmentMode should be set to .normal in order to "undimm" the underlying view otherwise it stays dimmed even when the sheet is dismissed (at least in a NavigationView context).

@schiewe schiewe changed the title Reset tintAdjustmentMode for presentationUndimmed Reset tintAdjustmentMode for presentationUndimmed Aug 31, 2022
@shaps80
Copy link
Owner

shaps80 commented Aug 31, 2022

Nice catch!

@shaps80 shaps80 merged commit 16b08f3 into shaps80:main Aug 31, 2022
@schiewe schiewe deleted the fix-undimmed-detents branch September 7, 2022 09:37
@shaps80
Copy link
Owner

shaps80 commented Sep 9, 2022

This is now in release v1.7.0 👍 – thanks for the contribution ;)

FYI: Your implementation had issues that I didn't notice until I tested it more completely. I've decided on a new API altogether.

presentationUndimmed is now deprecated in favour of: presenatationDetents(_:selection:largestUndimmedDetent:)

This basically keeps all the detent API together and allowed me to properly resolve the dimming issue. The interactive Demo project can be used to see it in action, just toggle the passthrough switch :)

@schiewe
Copy link
Contributor Author

schiewe commented Sep 13, 2022

@shaps80 Nice work! 💪 Thanks for the update. What were the issues that you encountered with the 1.7.0 solution?

I see some issues with the 1.8.2 API regarding compatibility with the original backported iOS 16 API to have this library a drop-in solution:

  1. presentationDetents(_ detents:) sets largestUndimmed: .large which is not the iOS 16 default. iOS 16 does not undim the underlying view at all. I think this was the reason presentationUndimmed(from:) was introduced in the first place. If you want to go this route I'd probably also add an optional largestUndimmedDetent parameter to this modifier default initialized with nil as for presentationDetents(_:selection:largestUndimmedDetent:).
  2. Docs missing for the largestUndimmedDetent parameter of presentationDetents(_:selection:largestUndimmedDetent:).
  3. I am not sure if it is a good idea in general to change the original API that is intended to be backported even with additional optional arguments. I'd prefer to use the original API if available. Having the additional options beyond the scope of the original backported API in a separate modifier makes it easier to use the additional functionality in combination with the original API, e.g., using .backport.presentationUndimmed(from:) together with the non-backported version of .presentationDetents(_:) even when branched directly in .backport.presentationDetents(_:). I am also planning to expose prefersScrollingExpandsWhenScrolledToEdge so that it can be set to false...

@shaps80
Copy link
Owner

shaps80 commented Sep 13, 2022

  1. presentationDetents(_ detents:) sets largestUndimmed: .large which is not the iOS 16 default.

This is true but when it's in the large position that value is ignored anyway and this approach avoided an unnecessary optional. It works fine afaict but if you come across a bug I would happily change it. So far seems to work well across all iOS versions for me.

  1. Docs missing for the largestUndimmedDetent parameter of presentationDetents(_:selection:largestUndimmedDetent:).

Nice catch! Will add 👍

For your last point about changing original API I'll create a dedicated issue so we can discuss in more detail.

#26

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.

None yet

2 participants