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

feat(messaging, apple): allow system to display button for in-app notification settings #13484

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

Conversation

StanleyCocos
Copy link
Contributor

@StanleyCocos StanleyCocos commented Oct 10, 2024

Description

I found that there is no option to set a button to display the app settings in the notification settings when registering for push notifications. I hope to add this feature.

resolves #13688

Specifically, the UNAuthorizationOptions.UNAuthorizationOptionProvidesAppNotificationSettings provided by iOS is missing.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@Lyokone
Copy link
Contributor

Lyokone commented Oct 29, 2024

Hey 👋 Thanks a lot for the PR, and sorry for the late review, it looks really good.

Lyokone
Lyokone previously approved these changes Oct 29, 2024
@StanleyCocos
Copy link
Contributor Author

Thank you Already submitted

@StanleyCocos
Copy link
Contributor Author

@Lyokone
Thank you
What else do I need to do to merge this request?

@Lyokone
Copy link
Contributor

Lyokone commented Nov 5, 2024

Hello @StanleyCocos, a lot of CI are currently red, you would need to apply proper formatting to your PR (melos run format-ci) then check the different actions to get it merged

@StanleyCocos
Copy link
Contributor Author

Thank you.
I will revise and follow up in time.

@StanleyCocos
Copy link
Contributor Author

@Lyokone
Hello, are you still continuing with the review?

@StanleyCocos
Copy link
Contributor Author

@russellwheatley Could you please guide me? It seems that my request isn't getting approved due to CI issues. I'm unsure if any adjustments are still needed on my part.
Thank you

@StanleyCocos
Copy link
Contributor Author

It seems like everyone is quite busy. However, I still hope to get some help. What I know is that there are CI issues, but I’m unsure how to resolve them. Can I get some guidance? I’m following up almost every day.

@StanleyCocos StanleyCocos requested a review from Lyokone December 10, 2024 05:34
@russellwheatley russellwheatley changed the title feat(messaging, ios): allow system to display button for in-app notification settings feat(messaging, apple): allow system to display button for in-app notification settings Dec 11, 2024
@russellwheatley
Copy link
Member

russellwheatley commented Dec 11, 2024

@StanleyCocos - Sorry for the late response. This looks good to me. I've approved CI run, let's see what the result is 👍

@StanleyCocos
Copy link
Contributor Author

@russellwheatley
Thank you for your response. I’ve adjusted the test, but there still seems to be an e2e issue, and I’m not sure what to do. Could you provide some guidance?

@russellwheatley
Copy link
Member

@StanleyCocos - I've just merged with main branch which ought to have a fix for CI. Nothing on your end to do now 🙏

@StanleyCocos
Copy link
Contributor Author

Thank you.
It seems there’s still an issue. To be honest, I might not have a deep understanding of CI. If you could provide some direction, I would really appreciate it.

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.

Added Allow system to show in-app notification settings button parameter
3 participants