-
Notifications
You must be signed in to change notification settings - Fork 346
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
[How to guides] Dealing with messages #980
Conversation
vasvi-sood
commented
Jun 25, 2021
•
edited
Loading
edited
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.
Beside the comments below, I was expected to see instructions/ steps instead of only code snippets without any explanation.
docusaurus/docs/howto/message.md
Outdated
## Mention | ||
|
||
Only a message in the room can mention(@) others. | ||
Mention messages are sent automatically by the bot if the message recieved as a parameter by the onMessage belongs to a room. The bot can mention(@) others. |
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.
Could you rewrite this sentence? There are so many conjunctions inside and should better be divided. It's a bit difficult to read.
Please note:
- Put the purpose or condition clauses at the beginning.
onMessage
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.
Okay, I will do the necessary changes.
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.
Done, Kindly review it now
docusaurus/docs/howto/message.md
Outdated
## Self message | ||
|
||
Self messages are sent automatically by the bot if the message recieved as a parameter by the onMessage function has also been sent by the bot itself. |
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.
same as above. Please rewrite this sentence.
You could either do like how tutorial team does (#985) or like JavaScript. Basically I would suggest you to add some sentences to explain the code snippets. Please let me know how you think is better? Thank you. |
I think following the pattern that the tutorials team is following will be more intuitive. I wll do it that way. |
or you could see how @abhishek-iiit did, he figured out quite a good way. #981 |
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'm not sure did you make any change?
Maybe you could explain what the routines are or where we could find the explanation(ex. .mentionList()).
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.
Correct the spelling of Received at all places.
I have not made all the required changes. I'll do that soon. |
@proudofsimin and @Rohitesh-Kumar-Jain I have made all the necessary changes. Kindly review the same. |
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.
Changes as suggest in #1011, else LGTM :)
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.
Thanks for the improvements.
Please follow my reviews.
docusaurus/docs/howto/message.md
Outdated
@@ -180,14 +202,17 @@ bot.on('message', onMessage) | |||
```js | |||
const { Message } = require('wechaty') |
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.
import { Message } from 'wechaty'
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.
Thanks for implementing the suggested changes : )
Hi @vasvi-sood Please resolve the conflict first. |
done |
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
@abhishek-iiit pls resolve conflicts first :) |
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
@lijiarui You have blocked this PR for months. I'll merge it in favor of |