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

feature(): Phonegap local notifications #1474

Merged
merged 16 commits into from
May 9, 2017
Merged

feature(): Phonegap local notifications #1474

merged 16 commits into from
May 9, 2017

Conversation

danielsogl
Copy link
Owner

@danielsogl danielsogl commented May 5, 2017

fix #1417

Daniel Sogl and others added 5 commits April 30, 2017 19:22
In some plugins the typescript markup was missing.
I also unified the console.log string from console.log("hello") to console.log('Hello') so any plugin page look the same.
@danielsogl danielsogl changed the title Phonegap local notifications feature(): Phonegap local notifications May 5, 2017
@ihadeed
Copy link
Collaborator

ihadeed commented May 5, 2017

Oh you picked a tricky plugin to be your first 😁 ... I personally think the plugin is poorly made (the actual plugin by phonegap).

The way this wrapper needs to work is:

declare var Notification: any;
// can use a shorter name here, like PLNObject ?
export class PhonegapLocalNotificationObject {

  private _objectInstance: any;

  // instance properties can go here, see https://github.com/phonegap/phonegap-plugin-local-notification/blob/master/www/notification.js#L23-L33
  @CordovaProperty title: string;
  @CordovaProperty dir: string;
  @CordovaProperty lang: string;

  constructor(title: string, options: any) {
    if (checkCordovaAvailablity('Notification') === true) {
      // if _objectInstance doesn't exist, nothing with the @CordovaInstance or @CordovaProperty decorator will work
      this._objectInstance = new Notification(title, options);
    }
  }

  @CordovaInstance({ sync: true })
  close(): void { }

}

@Plugin({
  ...
})
@Injectable()
export class PhonegapLocalNotification {

  create(title: string, options: any) { return new PhonegapLocalNotificationObject(title, options); }

  @Cordova()
  requestPermission(): Promise<any> { return; }

}

@danielsogl
Copy link
Owner Author

Thank you for your quick reply.

Can you explain how I can call the checkCordovaAvailablity method?

@ihadeed
Copy link
Collaborator

ihadeed commented May 5, 2017

@danielsogl

Sorry it's checkAvailability not checkCordovaAvailabilty ... and it's exported by @ionic-native/core

Here's an example: https://github.com/driftyco/ionic-native/blob/master/src/%40ionic-native/plugins/google-maps/index.ts#L64

@danielsogl
Copy link
Owner Author

Okay fine. I changed it.

Can you check the build errors so I or you can check this plugin?

* @param title {string} Title of the local notification.
* @param Options {LocalNotificationOptions} An object containing optional property/value pairs.
*/
@Cordova()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to remove this decorator because it will override the functionality you wrote

/**
* closes an open notification.
*/
close(): void { }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the close() method from here. It's an instance method, you already got it in PLNObject.

See https://github.com/phonegap/phonegap-plugin-local-notification/blob/master/www/notification.js#L80

* ```
*/
@Plugin({
pluginName: 'Phonegap Loca Notifications',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Got a typo here.. Loca -> Local

import { Cordova, CordovaInstance, Plugin, IonicNativePlugin, checkAvailability } from '@ionic-native/core';

declare var Notification: any;
// can use a shorter name here, like PLNObject ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove comment

@ihadeed
Copy link
Collaborator

ihadeed commented May 5, 2017

@danielsogl what build errors?

}

/**
* @name phonegap-local-notifications
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name needs to be Phonegap Local Notification ... also directory name should be phonegap-local-notification

@danielsogl
Copy link
Owner Author

Thank you again.

I talked about the build errors when I run npm run build.

@ihadeed ihadeed merged commit 3d747d3 into danielsogl:master May 9, 2017
@danielsogl danielsogl deleted the phonegap-local-notifications branch May 14, 2017 14:15
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.

feature(local notifications): offical phonegap plugin released
2 participants