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.

Sign up for GitHub

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

Jump to bottom

Add constants for notification importance #1959

Merged
merged 8 commits into from
Apr 23, 2021
Merged

Add constants for notification importance #1959

merged 8 commits into from
Apr 23, 2021

Conversation

Mookiies
Copy link
Contributor

@Mookiies Mookiies commented Apr 21, 2021 •
edited
Loading

Constants come from: https://developer.android.com/reference/android/app/NotificationManager#IMPORTANCE_DEFAULT

Adding these constants as an export would allow users of the library to have clearer code without floating numbers that require going to look at documentation to see what they mean.

This code

PushNotification.createChannel(
      {
        channelId: "default-channel-id", // (required)
        channelName: `Default channel`, // (required)
        channelDescription: "A default channel", // (optional) default: undefined.
        soundName: "default", // (optional) See `soundName` parameter of `localNotification` function
        importance: 4, // (optional) default: 4. Int value of the Android notification importance
        vibrate: true, // (optional) default: true. Creates the default vibration patten if true.
      });

would become:

import PushNotification, {Importance} from 'react-native-push-notification';
...
createDefaultChannels() {
  PushNotification.createChannel(
    {
      channelId: "default-channel-id", // (required)
      channelName: `Default channel`, // (required)
      channelDescription: "A default channel", // (optional) default: undefined.
      soundName: "default", // (optional) See `soundName` parameter of `localNotification` function
      importance: Importance.DEFAULT, // (optional) default: Importance.HIGH. Int value of the Android notification importance
      vibrate: true, // (optional) default: true. Creates the default vibration patten if true.
    },

Boris Tacyniak and others added 6 commits March 29, 2021 09:22
Document: Override on onNewIntent
2e2226d
@isamu
Documentation update, fix indentation
f031ce7 
Fix: indentation of Android code.
Merge pull request zo0r#1941 from isamu/patch-1
86d562e 
Documentation update, fix indentation
Update README.md
49619e8
@maheshwarimrinal
Adding mavenCentral() as jcenter() is shutting
64c1dff 
Adding mavenCentral() as jcenter() is shutting down
Merge pull request zo0r#1956 from maheshwarimrinal/patch-1
b77c465 
Adding mavenCentral() as jcenter() is shutting
@Mookiies
Copy link
Contributor Author

Is this a change that you'd be willing to support?

I'd be happy to update documentation as a part of this PR but didn't want to go through the effort if this is a change that is undesired.

@Mookiies Mookiies marked this pull request as ready for review April 21, 2021 22:37
@Dallas62
Copy link
Collaborator

Hi @Mookiies
Thanks for this PR !
I think this can help other developers, when you think it's ready, I will review it and merge it 😉
Regards,

@Mookiies Mookiies force-pushed the addConstants branch 2 times, most recently from 17712b0 to e61a5ef Compare April 22, 2021 18:04
@Mookiies
Copy link
Contributor Author

Hey @Dallas62 I've updated the documentation and the PR is ready for review 😃

Dallas62
Dallas62 reviewed Apr 23, 2021
View reviewed changes
index.js Outdated
@@ -604,4 +604,14 @@ Notifications.setNotificationCategories = function() {
return this.callNative('setNotificationCategories', arguments);
}

// https://developer.android.com/reference/android/app/NotificationManager#IMPORTANCE_DEFAULT
Notifications.IMPORTANCE = Object.freeze({
Copy link
Collaborator

Choose a reason for hiding this comment

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

A small change to follow the current case would be to rename IMPORTANCE to Importance (ucfirst).
For constant (MIN, LOW, ....) this is OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done

Dallas62
Dallas62 reviewed Apr 23, 2021
View reviewed changes
README.md Outdated
"unspecified" = NotificationManager.IMPORTANCE_UNSPECIFIED
IMPORTANCE.DEFAULT = NotificationManager.IMPORTANCE_DEFAULT\
IMPORTANCE.HIGH = NotificationManager.IMPORTANCE_HIGH\
IMPORTANCE.LOW = NotificationManager.IMPORTANCE_LOW\
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is some \ at the end of some line, is there any reason ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the \ adds a line break in markdown, making it easier to read.

Before:
Screen Shot 2021-04-23 at 10 30 03 AM

After:
Screen Shot 2021-04-23 at 10 30 18 AM

Dallas62
Dallas62 requested changes Apr 23, 2021
View reviewed changes
Copy link
Collaborator

@Dallas62 Dallas62 left a comment

Choose a reason for hiding this comment

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

Hi @Mookiies
I just check the PR, I don't see issue just format 😉
Thanks again for this PR !

@Mookiies Mookiies requested a review from Dallas62 April 23, 2021 17:38
@Mookiies
Copy link
Contributor Author

@Dallas62 Made the rename change. Thanks for the quick review 😃

@Dallas62
Copy link
Collaborator

Hi @Mookiies
Thanks for your changes !
I will take time on it next week, I will release it with another bug fix,
Regards

@Dallas62 Dallas62 changed the base branch from master to dev April 23, 2021 19:24
@Dallas62 Dallas62 merged commit 70c43d5 into zo0r:dev Apr 23, 2021
@Mookiies Mookiies deleted the addConstants branch April 23, 2021 19:59
@Dallas62 Dallas62 mentioned this pull request May 12, 2021
Closed
@Dallas62
Copy link
Collaborator

Hi @Mookiies
Your PR is now released 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dallas62 Dallas62 Awaiting requested review from Dallas62

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mookiies @Dallas62 @isamu @maheshwarimrinal