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
chore: converted notifications category from JS to TS #11025
Conversation
ce109c6
to
df2b27e
Compare
This pull request fixes 2 alerts when merging df2b27e into ac31ec0 - view on LGTM.com fixed alerts:
|
df2b27e
to
24f7b23
Compare
This pull request fixes 2 alerts when merging 24f7b23 into ac31ec0 - view on LGTM.com fixed alerts:
|
24f7b23
to
4c65fa7
Compare
This pull request fixes 2 alerts when merging 4c65fa7 into ac31ec0 - view on LGTM.com fixed alerts:
|
Codecov Report
@@ Coverage Diff @@
## dev #11025 +/- ##
==========================================
+ Coverage 48.19% 49.22% +1.02%
==========================================
Files 670 663 -7
Lines 32446 31779 -667
Branches 6598 6447 -151
==========================================
+ Hits 15638 15643 +5
+ Misses 15188 14650 -538
+ Partials 1620 1486 -134
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Some minor modifications are added as comments
packages/amplify-category-notifications/src/__tests__/channel-APNS.test.ts
Show resolved
Hide resolved
packages/amplify-category-notifications/src/__tests__/channel-FCM.test.ts
Outdated
Show resolved
Hide resolved
packages/amplify-category-notifications/src/__tests__/channel-FCM.test.ts
Outdated
Show resolved
Hide resolved
packages/amplify-category-notifications/src/__tests__/channel-APNS.test.ts
Outdated
Show resolved
Hide resolved
packages/amplify-category-notifications/src/notifications-manager.ts
Outdated
Show resolved
Hide resolved
a798fd6
This pull request fixes 2 alerts when merging a798fd6 into 262276a - view on LGTM.com fixed alerts:
|
packages/amplify-category-notifications/src/__tests__/channel-APNS.test.ts
Show resolved
Hide resolved
packages/amplify-category-notifications/src/commands/notifications.ts
Outdated
Show resolved
Hide resolved
packages/amplify-category-notifications/src/commands/notifications/add.ts
Show resolved
Hide resolved
packages/amplify-category-notifications/src/commands/notifications/add.ts
Show resolved
Hide resolved
a798fd6
to
90326ad
Compare
This pull request fixes 2 alerts when merging 90326ad into 262276a - view on LGTM.com fixed alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall solid refactor; just added some nit picks and some questions
packages/amplify-category-analytics/src/utils/pinpoint-helper.ts
Outdated
Show resolved
Hide resolved
packages/amplify-category-notifications/src/notifications-manager.ts
Outdated
Show resolved
Hide resolved
packages/amplify-category-notifications/src/notifications-manager.ts
Outdated
Show resolved
Hide resolved
packages/amplify-category-notifications/src/notifications-manager.ts
Outdated
Show resolved
Hide resolved
packages/amplify-category-notifications/src/notifications-types.ts
Outdated
Show resolved
Hide resolved
90326ad
to
07813a8
Compare
This pull request fixes 2 alerts when merging 07813a8 into 262276a - view on LGTM.com fixed alerts:
|
Description of changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.