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.
@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
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.
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 🤖
| 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.
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.
@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.
🗣️ 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[ ])