-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add support for block suggestion handlers #116
Conversation
Codecov Report
@@ Coverage Diff @@
## main #116 +/- ##
==========================================
+ Coverage 98.45% 98.56% +0.10%
==========================================
Files 46 47 +1
Lines 1753 1875 +122
Branches 93 105 +12
==========================================
+ Hits 1726 1848 +122
Misses 25 25
Partials 2 2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
const apiResp = await fetch("https://motivational-quote-api.herokuapp.com/quotes"); | ||
const quotes = await apiResp.json(); | ||
console.log('Returning', quotes.length, 'quotes'); | ||
const opts = { |
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 forgot the exact number but we may want to mention the maximum number of options/option_groups here for more developer-friendliness
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 couldn't find a reference to what is the maximum number of options one can return... I did end up posting in our api docs channel about this issue: https://slack-pde.slack.com/archives/C8DPMVAQY/p1666275726696799
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.
Extremely well documented in md and in jsdoc with great test coverage. Thank you Fil! Left a couple questions/musings and nits, but nothing blocking.
@@ -0,0 +1,160 @@ | |||
import type { BlockAction, BlockElement } from "./block_kit_types.ts"; |
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.
nit: any reason this file name is block_suggestion_types
while we just use suggestion_router*
elsewhere?
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.
Not really! Do you have a preference?
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.
Just to keep it consistent. I'd probably search for block_suggestion_*
first, but no strong opinion
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.
@shapirone should I then rename the action_router to block_action_router, too?
export type BlockSuggestionBody = | ||
& BlockAction // adds block_id and action_id properties | ||
& { | ||
// type: "block_suggestion"; |
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.
nit: I think we can remove this. I'm not sure there's much value in strictly typing this.
…a a addBlockSuggestionHandler API.
…her array of options or option_groups. Start of adding more JSDoc comments to older handlers.
…at is a natural next step for how to build upon external data source select menus.
… block_suggestion_router.
8b8173d
to
114cf9e
Compare
Summary
This PR adds a new method available to app functions:
addBlockSuggestionHandler
.It is a copy-paste of the existing Block Actions handler support, basically.
After merging this, we will have in place all of the available interactivity handlers. Thus, after this gets merged would be a good opportunity to review all the interactivity code and see how we should refactor it.
Testing
import_map.json
to the location of this checked out branch +/src/
, e.g.file:///Users/fmaj/src/deno-slack-sdk/src/
action_id
: