Delete same notification if enter the channel. #476
Conversation
hyochan
added
🎯 feature
Codecov Report
@@ Coverage Diff @@
## master #476 +/- ##
=======================================
Coverage 73.11% 73.11%
=======================================
Files 65 65
Lines 1257 1257
Branches 142 142
=======================================
Hits 919 919
Misses 338 338
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
@JongtaekChoi @geoseong Could you kindly review this work? |
| const parsedNotificationData = JSON.parse( | ||
| notification.request.content.data.data as string, | ||
| ); |
There was a problem hiding this comment.
compared to other similar codes like L517-520 in this file and L131-L134 in MainStackNavigator, this has only different style. do you have any special reason?
I think this kind of code style has to become the same.
There was a problem hiding this comment.
I changed like you commented, but I think it went back to original. I'll fix it.
There was a problem hiding this comment.
I changed like you commented, but I think it went back to original. I'll fix it.
I checked your commit, but no change was detected. please check if you'd pushed the commit to your repo.
and your other code in the same file L517-L529 looks similar to this code block. so could you tell me why do you think it went back to original? and also I don't know what is the original 😆.
please understand me if I ask the dumb question 🙏
There was a problem hiding this comment.
This change looks great. However, CodeCov bots warn you because no test code has been added. How about adding a few simple tests to this pool request? If you add 'expo-notification' mock to Message.test.tsx file you can easily raise up code coverage.
It's not necessary to add a test to this PR. I think we can make another PR and add it. And this is not mandatory, so you don't have to do it.
@hyochan if you think that PR looks good, I think you can merge this |
Delete notification if user is in channel, but not alert.
hyochan
force-pushed
the
feature/delete_same_notification
branch
from
2a6e6ec
to
6ad2efa
Compare
|
@all-contributors Please add @SimYunSup for code. |
|
@SimYunSup already contributed before to code |
Specify project
client - RN.
Description
Delete same notification if enter the channel.
And if user on the channel, delete the related notification.
Screen_Recording_20210831-124838_Expo.Go.mp4
Related Issues
issue #463
Checklist
Before you create this PR confirms that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.yarn lint && yarn tscyarn testoryarn test -uif you need to update snapshot.[WeHack] - [we go]