-
Notifications
You must be signed in to change notification settings - Fork 679
feat(web-api): add markdown_text property to chat.{postEphemeral|postMessage|scheduleMessage|update} methods #2330
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2330 +/- ##
==========================================
+ Coverage 92.74% 92.75% +0.01%
==========================================
Files 38 38
Lines 10660 10676 +16
Branches 687 692 +5
==========================================
+ Hits 9887 9903 +16
Misses 761 761
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@hello-ashleyintech This is looking great! Would it be possible to include this in a few other methods too to match changes of slackapi/python-slack-sdk#1718?
Edit: Oops keyboard shortctus are difficult - sorry for closing this ahhhhgh. |
|
@zimeg great catch! 😄 I will update this expeditiously |
zimeg
left a comment
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.
@hello-ashleyintech Nice and so appreciated - this is working well for me! 👁️🗨️
An update to the PR title was made just above this but before merging it, the fallback text warning might be nice to update to expect markdown_text too: 🙏 ✨
node-slack-sdk/packages/web-api/src/WebClient.ts
Lines 997 to 998 in dc32203
| const isEmptyText = (args: Record<string, unknown>) => | |
| args.text === undefined || args.text === null || args.text === ''; |
I left a few other comments of jsdoc also and would love to hear your thoughts on such topic! 🔏
| /** @description Accepts message text formatted in markdown. This argument should not be used | ||
| * in conjunction with blocks or text. Limit this field to 12,000 characters. */ |
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.
praise: Thank you for adding jsdoc 📚 ✨
thought: Might leaving line breaks to the editor should be an overall change we make? I'm thinking we could perhaps snag this from the kind docs.slack.dev/reference/methods pages in neartimes 🤖
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.
| reply_broadcast?: boolean; | ||
| /** @description Accepts message text formatted in markdown. This argument should not be used | ||
| * in conjunction with blocks or text. Limit this field to 12,000 characters. */ | ||
| markdown_text?: string; |
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.
praise: I am a fan of inlining these arguments! 👾
|
Hey guys, I'm happy to help in order to expedite this release, given the branch isn't from a fork. @zimeg by adjusting the fallback to consider the markdown fields, do you mean: const isEmptyText = (args: Record<string, unknown>) =>
args.text === undefined || args.text === null || args.text === '' || args.markdown_text === undefined || args.markdown_text === null || args.markdown_text === ''; |
|
Apparently I don't have permission to push to this branch directly, sorry about that 😢 |
zimeg
left a comment
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.
@hello-ashleyintech A few final touches on this PR were made in recent commits! After more testing I'm feeling good to merge this 🚢 💨
@pedropaccola Thanks too for sharing this suggestion! A similar change was made in 46359c6 that also adds tests. The investigation is so appreciated 🙏 ✨
| // 3. channel and attachments | ||
| type MessageContents = ChannelAndText | ChannelAndBlocks | ChannelAndAttachments; | ||
| // 4. channel and markdown_text | ||
| type MessageContents = ChannelAndText | ChannelAndBlocks | ChannelAndAttachments | ChannelAndMarkdownText; |
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.
🗣️ note: Including markdown_text here was found to be helpful in avoiding typescript errors since none of the other "content" arguments are required!
⏳ ramble: Otherwise an attachments was expected I noticed-

Summary
This PR resolves issue #2329 and adds an optional string
markdown_textproperty for ChatPostMessageArgumentsRequirements (place an
xin each[ ])