-
Notifications
You must be signed in to change notification settings - Fork 677
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 |
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.
@@ -265,4 +274,7 @@ export type ChatUpdateArguments = MessageContents & { | |||
file_ids?: string[]; | |||
/** @description Broadcast an existing thread reply to make it visible to everyone in the channel or conversation. */ | |||
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 😢 |
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 🙏 ✨
// Models message-creation arguments, user must provide one of the following combinations: | ||
// 1. channel and text | ||
// 2. channel and blocks | ||
// 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_text
property for ChatPostMessageArgumentsRequirements (place an
x
in each[ ]
)