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

Replace the method to show SFSafariViewController #45

Merged
merged 11 commits into from Mar 21, 2024

Conversation

touyou
Copy link
Contributor

@touyou touyou commented Mar 20, 2024

What

I replace the method to show SFSafariViewController because I found some usability problem of current methods.
This is how the interaction has changed.

Before

480pB.mov

After

480pA.mov

Why

I found these problems:

  • SFSafariViewController enables users to close by edge swipe, so users can close sheet of Safari by edge swipe.
  • When scrolling web page, top toolbar get weird. This is because SFSafariViewController cannot recognize top toolbar height inside sheet correctly.

How

I'm TCA beginner, so sorry for implementing it by UIApplication extension functions.
I replaced these view:

  • ScheduleFeature
    • Detail
    • Schedule ← Revert it
  • trySwiftFeature
    • Acknowledgements
    • Profile
    • trySwift

And not replace Sponsors because it already uses fullScreenCover.
According to this article, fullScreenCover still has usability problem, but it's not critical and I decided not to replace it.

@d-date
Copy link
Member

d-date commented Mar 20, 2024

HI @touyou ,

Thank you for improving SF Safari experience.
It might be create Safari dependency value instead of using Safari module.

See how it work and cherry-pick my commit to improve your pull request.
https://github.com/tryswift/trySwiftTokyoApp/tree/safari

@d-date
Copy link
Member

d-date commented Mar 20, 2024

This might be help for you to check diff. I have no plan to merge my PR. Please try to complete your work!
#48

@touyou
Copy link
Contributor Author

touyou commented Mar 20, 2024

@d-date Thanks for very helpful commit and advice!
I try to complete it tonight or tomorrow!

Copy link
Member

@d-date d-date left a comment

Choose a reason for hiding this comment

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

Thank you!

@d-date d-date merged commit ba95e2e into tryswift:main Mar 21, 2024
@touyou touyou deleted the fix/sfsafariview_navigation branch March 21, 2024 01:29
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