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.

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 #1237 Updates to message subtypes doc #1240

Merged
merged 5 commits into from
Dec 10, 2021
Merged

Fix #1237 Updates to message subtypes doc #1240

merged 5 commits into from
Dec 10, 2021

Conversation

wongjas
Copy link
Member

@wongjas wongjas commented Dec 8, 2021

Summary

Fixes #1237 for both EN and JP docs. Changes message subtype from bot_message to message_changed and makes it explicit to import subtype from the Bolt package.

Requirements (place an x in each [ ])

@wongjas wongjas requested review from seratch and srajiang December 8, 2021 01:26
@wongjas wongjas self-assigned this Dec 8, 2021
@seratch seratch added the docs M-T: Documentation work only label Dec 8, 2021
@seratch seratch added this to the 3.9.0 milestone Dec 8, 2021
Comment on lines 48 to 49
app.message(subtype('message_changed'), ({ event }) => {
console.log(`The user ${event.message.user} changed their message from ${event.previous_message.text} to ${event.message.text}`);
Copy link
Member

Choose a reason for hiding this comment

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

I know the usage of console is not your change but we may want to improve the part as well. Can you change the code this way? It's the same for the English one.

Suggested change
app.message(subtype('message_changed'), ({ event }) => {
console.log(`The user ${event.message.user} changed their message from ${event.previous_message.text} to ${event.message.text}`);
app.message(subtype('message_changed'), ({ event, logger }) => {
logger.info(`The user ${event.message.user} changed their message from ${event.previous_message.text} to ${event.message.text}`);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that makes sense. Maybe we should change it for the entirety of the docs? It looks like console.log is used throughout at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. In the case, let's work on the replacement in a different issue/PR!

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #1240 (a2a9ca9) into main (9658caa) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1240   +/-   ##
=======================================
  Coverage   73.22%   73.22%           
=======================================
  Files          17       17           
  Lines        1438     1438           
  Branches      431      431           
=======================================
  Hits         1053     1053           
  Misses        300      300           
  Partials       85       85           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9658caa...a2a9ca9. Read the comment docs.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Looks good to me. @srajiang if this looks good to you too, can you "squash and merge" this PR tomorrow in your local time?

@wongjas
Copy link
Member Author

wongjas commented Dec 9, 2021

Conflicts have been resolved ✅

@seratch seratch merged commit bd016c1 into slackapi:main Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs M-T: Documentation work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update example code for subtype() middleware docs
2 participants