Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

Added Channel Group #2607

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Conversation

OliverOng1995
Copy link

@OliverOng1995 OliverOng1995 commented Nov 1, 2018

Description

In Android O,Push Channel also have group to categorize notification channel such as Work and Personal Notifications

Motivation and Context

It can help developer to assign notification channel to a group,easily categorizing it.
Which help user experience better.
Example use cases are chat app between Personal and Business account messages notifications

How Has This Been Tested?

Using Outsystem,I use the Firebase Push Plugin and modified the source code and the wrapper on the plugin on Outsystem itself.Also reference the source code instead of the original author git with the channels related function in the latest version of this plugin.
Also tested on Mi Max on lineageOS 15.1 and Mi A1 on Android 8.0 for creation,listing and deletion of channel.Setting and listing channels with the group

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Added group functionality
with createChannelGroup,deleteChannelGroup,ListChannelGroup
createChannel can add channel group created and listChannel can show the channel group that channel belong to
@fredgalvao
Copy link
Collaborator

Thanks for contributing!

Can you update the type definitions to reflect on those new APIs and attributes?

@OliverOng1995
Copy link
Author

What is it,is it the API.md

@fredgalvao
Copy link
Collaborator

It's the https://github.com/phonegap/phonegap-plugin-push/blob/master/types/index.d.ts file. That typescript "interface descriptor" needs to reflect the new APIs you're adding, so that the project compiles for the users that consume our API through a typescript project.

Also added comments and clean up code to match the coding style
@OliverOng1995
Copy link
Author

There you have it

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated
/**
* Set notitfication visibility
*/
visibility: integer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same integer vs number comment from above.

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated
/**
* Set Channel Group Name
*/
name: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same "can these be optional" suggestion applies to these attributes.

Copy link
Author

Choose a reason for hiding this comment

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

It is not optional

*/
id: string
/**
* camillebeaumont name and description fix
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these notes to fix later? We don't have the name attribute yet, so we don't have an official doc to copy from, so it would make sense.

Copy link
Author

Choose a reason for hiding this comment

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

Yup based on a previous pull request

types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
fredgalvao and others added 4 commits November 2, 2018 08:57
Co-Authored-By: OliverOng1995 <[email protected]>
Co-Authored-By: OliverOng1995 <[email protected]>
Co-Authored-By: OliverOng1995 <[email protected]>
Co-Authored-By: OliverOng1995 <[email protected]>
types/index.d.ts Outdated Show resolved Hide resolved
Co-Authored-By: OliverOng1995 <[email protected]>
types/index.d.ts Outdated Show resolved Hide resolved
Co-Authored-By: OliverOng1995 <[email protected]>
types/index.d.ts Outdated Show resolved Hide resolved
@OliverOng1995
Copy link
Author

OliverOng1995 commented Nov 2, 2018

Sorry I am not familiar with typescript variable,sorry to inconvenience you with my new features that I want to add on.I thought it seemed like javascript

fredgalvao and others added 3 commits November 2, 2018 09:06
Co-Authored-By: OliverOng1995 <[email protected]>
Co-Authored-By: OliverOng1995 <[email protected]>
Co-Authored-By: OliverOng1995 <[email protected]>
types/index.d.ts Outdated Show resolved Hide resolved
Co-Authored-By: OliverOng1995 <[email protected]>
types/index.d.ts Outdated Show resolved Hide resolved
fredgalvao and others added 2 commits November 2, 2018 09:07
Co-Authored-By: OliverOng1995 <[email protected]>
Co-Authored-By: OliverOng1995 <[email protected]>
types/index.d.ts Outdated Show resolved Hide resolved
Co-Authored-By: OliverOng1995 <[email protected]>
@fredgalvao
Copy link
Collaborator

Sorry I am not familiar with typescript variable,sorry to inconvenience you with my new features that I want to add on.I thought it seemed like javascript

Not at all an inconvenience! Please pat yourself on the back, you're doing all of us a big favor.

You even walked the extra mile to document/type missing parts that weren't even related to your additions to the API, which is awesome.
The simple fact that you're taking the time to work through suggestions on this PR is another amazing things. So yes, thank you very much for your contribution ;)

I won't bother you with some of the other missing parts I noticed, as they're definitely not related to your PR, and I'll work on them later myself.

@fredgalvao
Copy link
Collaborator

The type definitions, as they are right now, are fine. Missing bits will be considered my responsibility soon.

@macdonst or someone else should review the native part soon.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants