-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix recall message #84
Conversation
Why did you close the original PR #81? All the discussion is over there, closing that caused we lost all the conversations. |
Because of I made some mistakes when I updated this PR. So I close that PR and push a new one. I think we can discuss in the Issues? |
It's not good practice if you create a new PR in order to replace an old PR. The better practice should be continuing to modify the original PR until it had been merged. Link to wechaty/wechaty#1728 |
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.
There are three comments added, and from my perspective are unnecessary. But that is not a blocker for this code to be merged. The main logic looks good to me. So approve this change.
@@ -52,7 +52,9 @@ export function webMessageType ( | |||
*/ | |||
case WebMessageType.SYS: | |||
return MessageType.Text | |||
|
|||
// add recall type |
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.
I think we don't need the comment here, since the type is clearly stated it's purpose.
@windmemory Thanks for the approvement. @wxul Thank you very much for the contribution, you did a great job! |
Related to this PR, some code has been changed