Skip to content

Include the system notification content height in verticalOffset #15

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

briancordanyoung
Copy link

@briancordanyoung briancordanyoung commented May 16, 2024

When dynamic type becomes excessively large, the system notification view may expand and become visible while in the dismissed state. The verticalOffset of 250 is not enough to keep the view off screen.

This PR includes the height of the system notification view content in the offset ensuring that the view is not visible in these cases.

It's not clear to me, but this might allow for the base offset of 250 to be reduced.

@danielsaidi
Copy link
Owner

Hi @briancordanyoung

Thank you - looks promising! I will give it a try, and as you say, hopefully it will let us reduce or remove the base offset.

@danielsaidi danielsaidi deleted the branch danielsaidi:main October 6, 2024 12:39
@danielsaidi danielsaidi closed this Oct 6, 2024
@briancordanyoung
Copy link
Author

briancordanyoung commented Oct 6, 2024

@danielsaidi I see this PR was rejected. Is there anything I could do to improve it? Do you have another technique you intend to use to prevent large text sizes from making the "offscreen" view visible?

@danielsaidi
Copy link
Owner

Hi @briancordanyoung

Sorry about this, I must have closed it accidentally. I've been working on most of my open-source projects in parallel to get them ready for Swift 6, and have been spreading myself a bit thin. I managed to close another issue as well 🙈

Thank you for letting me know! I'll reopen it and will take a look at it once I'm done with the Swift 6 transition.

@danielsaidi danielsaidi reopened this Oct 7, 2024
@danielsaidi danielsaidi changed the base branch from master to main October 7, 2024 10:10
@danielsaidi
Copy link
Owner

@briancordanyoung I think was auto-closed as a result of me renaming the master branch to main yesterday. 🫠

@danielsaidi
Copy link
Owner

Hi @briancordanyoung

I'm going through all my open-source projects and look at PRs that have gone unmerged for a while.

Do you still use/need this in the library?

@briancordanyoung
Copy link
Author

Hi! The library is still in use but I have left the company.

@briancordanyoung
Copy link
Author

@tiffsaka, you may want to join in. This is the library that is used for the network offline notifications.

@danielsaidi
Copy link
Owner

Hi @briancordanyoung and @tiffsaka, any news on this or should I close?

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.

2 participants