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

remove background push and fix push response handling #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CR4567
Copy link
Collaborator

@CR4567 CR4567 commented Jan 11, 2023

No description provided.

@@ -33,17 +33,25 @@ public override async void DidReceiveNotificationRequest(UNNotificationRequest r
//Save the notification and create a mutable copy
BestAttemptContent = (UNMutableNotificationContent)request.Content.MutableCopy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow the formatting is really fucked up here with tabs and spaces.

@@ -51,22 +51,10 @@ public static INotifoMobilePush UseFirebasePluginEventsProvider(this INotifoMobi
/// <param name="notifo">The <see cref="INotifoMobilePush"/> instance.</param>
/// <param name="data">The notification data dictionary.</param>
/// <returns>A <see cref="Task"/> representing the result of the asynchronous operation.</returns>
public static async Task DidReceiveMessageAsync(this INotifoMobilePush notifo, NSDictionary data)
public static void DidReceiveMessageAsync(this INotifoMobilePush notifo, NSDictionary data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be called "Async", if it is not async.

{
options ??= new PullRefreshOptions();
var options = new PullRefreshOptions();
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the full options useless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah that's right. Could be removed completely but what do you think in general of removing the pullRefresh?

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I tried not to have some non-configurable values, but it is weird, that these options are configured over the method. So we should just add a method to INotifoPush like

NotifoPush.Current.SetPullOptions(...);

Copy link
Contributor

Choose a reason for hiding this comment

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

I have introduced this change in the base branch

Base automatically changed from do-not-cache-settings to master January 17, 2023 08:51
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