Skip to content
This repository was archived by the owner on Dec 18, 2019. It is now read-only.

Handling a Message, with code examples from showcase apps#43

Merged
josemigallas merged 5 commits into
aerogear:masterfrom
josemigallas:AEROGEAR-3007
Jun 15, 2018
Merged

Handling a Message, with code examples from showcase apps#43
josemigallas merged 5 commits into
aerogear:masterfrom
josemigallas:AEROGEAR-3007

Conversation

@josemigallas

@josemigallas josemigallas commented Jun 12, 2018

Copy link
Copy Markdown
Contributor

Motivation: https://issues.jboss.org/browse/AEROGEAR-3007

There are many problems with tagging the code in showcase examples.

  • In Android, the code is in a XML file so the comments have a different format than //.
  • In Cordova, the implementation is very attached to the framework and architecture of the showcase app (ionic) and it's not possible to keep the snippet brief and clear (see code).

@josemigallas josemigallas changed the title [WIP] Handling a Message, with code examples from showcase apps Handling a Message, with code examples from showcase apps Jun 12, 2018


[role="secondary"]
.iOS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jhellar could you evaluate if this is correct? I could use some help here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that you first need to handle remote notification with https://github.com/aerogear/ios-showcase-template/blob/master/ios-showcase-template/AppDelegate.swift#L116. NotificationCenter is not part of AGSPush. It can be used to post the notification (https://github.com/aerogear/ios-showcase-template/blob/master/ios-showcase-template/push/PushHelper.swift#L18-L20) to a View so that it can be rendered. @wtrocki can you please confirm?

@wtrocki wtrocki Jun 13, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not think that we should document parts that are not related to our SDK. Our SDK is not made to handle push notifications or anything related to push. The only purpose of the SDK is to:

Register application token to UPS server and in my opinion only this should be documented as rest is just classical apple story for push which changes so often and depends on os versions.

Same will apply to Android.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with @wtrocki that this is too big to take on, and we should skip it..
However, I disagree with the general point. It's ok to document any steps that user needs to perform in order to be successful with mobile services regardless of software origin... provided that documentation task is reasonable (and in this case, I don't think it is)

@wtrocki wtrocki Jun 13, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@finp Yeah, main problem here is that this code will outdate faster than yoghurt in the fridge and people will actually get better experience if we point them to:
https://developer.apple.com/notifications/

}
});

pushObject.on("notification")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is specific to ionic extension. Worth to mention that.

@josemigallas

Copy link
Copy Markdown
Contributor Author

OK I updated it. I am in favor of not describing parts that are out of our SDK, it's too volatile and actually up to each particular user.

@jhellar jhellar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One comment, otherwise LGTM.

****

The {SDK} for JavaScript does not provide a way to handle incoming messages. We recommend using the link:https://ionicframework.com/docs/native/push[Ionic Push plugin].
[source,javascript]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that this line can be removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@josemigallas
josemigallas merged commit fcec4e7 into aerogear:master Jun 15, 2018
@josemigallas
josemigallas deleted the AEROGEAR-3007 branch June 15, 2018 11:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants