-
Notifications
You must be signed in to change notification settings - Fork 657
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 focus_on_load property to views.* API responses #1386
Conversation
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.
LGTM, left some questions about if/how what we have on the API Docs site needs to be updated as a result.
@@ -28,6 +28,7 @@ export interface Team { | |||
default_channels?: string[]; | |||
is_verified?: boolean; | |||
enterprise_domain?: string; | |||
url?: 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.
I don't see any of enterprise_domain
, is_verified
nor url
described in the admin.teams.settings.info
API method example response. Should it be documented there too?
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.
Yes, we can improve it. Not only the missing fields but also the invalid type data like "default_channels": "array"
@@ -24,6 +24,7 @@ export interface Team { | |||
email_domain?: string; | |||
icon?: Icon; | |||
is_verified?: boolean; | |||
url?: 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.
I'm just comparing what we have in these responses to what's on the docs site, and for the teams.info
method, is_verified
and url
does not exist, but it also says that enterprise_id
and enterprise_name
exist. Should we add the enterprise_*
properties here, too, then, and should the is_verified
and url
properties be documented on the public site?
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.
Should we add the enterprise_* properties here, too,
Yes, we should. The reason why enterprise_* properties are not available in this class is that the Java SDK integration tests with team.info endpoint do with only a non-Grid workspace. I'll update the tests and this PR later on.
then, and should the is_verified and url properties be documented on the public site?
Yes, the example should be updated.
@@ -86,6 +86,7 @@ export interface Element { | |||
min_length?: number; | |||
max_length?: number; | |||
dispatch_action_config?: DispatchActionConfig; | |||
focus_on_load?: boolean; |
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.
Good news, was able to find this on the docs site here: https://api.slack.com/reference/block-kit/block-elements
Bad news, not on the API methods example response for views.open
: https://api.slack.com/methods/views.open - but I guess it would be too verbose to fully document all the possible Block elements on this page? What do you think?
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.
Yeah, I don't think the views.open
API method page should cover all the patterns of blocks. If we would like to do so, it should mention not only focus_on_load but also many things such as response_url_enabled, and dispatch_action_config. As the page has a link to the modal document, I don't think we should repeat all the things there.
Thanks for the comments on my questions, @seratch ! If you wish I can try to update the api.slack.com/methods documentation I commented on. Let me know! |
Summary
This pull request adds focus_on_load in the
views.*
API responses. Also,url
was added toteam
data recently.Requirements (place an
x
in each[ ]
)