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

iOS: Add support for userActivity API #623

Merged
merged 15 commits into from
Mar 23, 2018
Merged

iOS: Add support for userActivity API #623

merged 15 commits into from
Mar 23, 2018

Conversation

LeoNatan
Copy link
Contributor

At first will only include associated domain user activity, but will be extensible.

For this, I merged openURL() and sendUserNotification() internally to deliverPayload(). The new userActivity() API will use that same mechanism.

Closes #622

Also fixed #601 in this branch

@LeoNatan LeoNatan requested a review from rotemmiz as a code owner March 14, 2018 18:19
@@ -23,7 +23,7 @@ describe(':ios: User Notifications', () => {
await expect(element(by.text('From calendar'))).toBeVisible();
});

it('Foreground push notifications', async () => {
it.only('Foreground push notifications', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

you forgot this here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@rotemmiz rotemmiz changed the title Add support for userActivity API (WIP) [WIP] Add support for userActivity API Mar 14, 2018
@LeoNatan LeoNatan changed the title [WIP] Add support for userActivity API Add support for userActivity API (WIP) Mar 14, 2018
@LeoNatan LeoNatan changed the title Add support for userActivity API (WIP) [WIP] Add support for userActivity API Mar 14, 2018
@LeoNatan
Copy link
Contributor Author

Hm, something failed for RN53. Will look romorrow.

@@ -123,6 +113,20 @@ class Device {
return !params.delete && !params.newInstance && this._processes[_bundleId];
}

_assertHasSingleParam(singleParams, params) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the "assert" moniker incorrect if it has the side-effect of returning the count? Should be "count" and the throw should be in the calling method.

async _sendPayload(key, params) {
const payloadFilePath = this.deviceDriver.createPayloadFile(params);
let payload = {};
//JS does not support {key: "asd"} JSON generation where the `key` is a variable 🤦‍♂️
//JS does not support {key: "asd"} JSON generation where the `key` is a variable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Not done yet...

  • There's Android client to align to the protocol changes

@LeoNatan
Copy link
Contributor Author

If the tests fail, just restart the specific build that failed. I've been doing that for a few times. Travis…

@LeoNatan LeoNatan changed the title [WIP] Add support for userActivity API Add support for userActivity API Mar 22, 2018
@rotemmiz
Copy link
Member

Android is all good. When CI is green, let's merge

@@ -187,7 +184,6 @@ class Device {
async _sendPayload(key, params) {
const payloadFilePath = this.deviceDriver.createPayloadFile(params);
let payload = {};
//JS does not support {key: "asd"} JSON generation where the `key` is a variable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why was this removed? This line is important, otherwise I might be confused like before. There is nothing logical about JS.

Copy link
Member

Choose a reason for hiding this comment

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

Then let's write a manifest about everything that's broken in JavaScript, it's not gonna be a short one...

The tests protect this specific thing from a wrong change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this comment bothering you?

@rotemmiz rotemmiz changed the title Add support for userActivity API iOS: Add support for userActivity API Mar 23, 2018
@rotemmiz rotemmiz merged commit 792a782 into master Mar 23, 2018
@rotemmiz rotemmiz deleted the UserActivity branch March 23, 2018 10:24
@wix wix locked and limited conversation to collaborators Jul 23, 2018
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.

Support UserActivity createPushNotificationJson creates a notification.json file under a constant path
2 participants