Skip to content
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

Factor out confirm, placeholder, focus_on_load and dispatch_action_config properties #1550

Merged
merged 4 commits into from Oct 31, 2022

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Oct 28, 2022

The idea behind this PR is to have a set of interfaces that other Block Kit elements can extend from easily.

This PR also starts adding some of the documentation hints from api.slack.com as JSDoc comments.

It is a small refactor, but it is especially helpful when documenting properties, as then they can be documented in one place but used in many places.

Let me know what you think! Like/dislike/don't care?

…to own interfaces that other elements extend from.

Factor out `dispatch_action_config` property into own extendable interface.
@filmaj filmaj added question M-T: User needs support to use the project semver:patch enhancement M-T: A feature request for new functionality discussion M-T: An issue where more input is needed to reach a decision area:typescript issues that specifically impact using the package from typescript projects pkg:types applies to `@slack/types` labels Oct 28, 2022
@filmaj filmaj self-assigned this Oct 28, 2022
placeholder?: PlainTextElement;
}

export interface Dispatchable {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now only one element uses this interface, but if/when we merge #1549, then the new input elements like URL, email and number inputs will also use this interface.

Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 💯 this reminds me of a decorator pattern?

Copy link
Member

@srajiang srajiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this direction, and thank you for additional jsdoc to help document things. Left a minor comment about naming.

packages/types/src/index.ts Outdated Show resolved Hide resolved
dispatch_action_config?: DispatchActionConfig;
}

export interface UsersSelect extends Action, Confirmable, Focusable, Placeholderable {
Copy link
Member

@srajiang srajiang Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:chefs-kiss:

filmaj and others added 2 commits October 28, 2022 13:32
Co-authored-by: Sarah Jiang <sarahjiang@slack-corp.com>
Copy link
Member

@mwbrooks mwbrooks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3 Love it! Only found a minor improvement suggestion.

packages/types/src/index.ts Outdated Show resolved Hide resolved
Co-authored-by: Michael Brooks <mbrooks@slack-corp.com>
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These documentation hints are wonderful, particularly when @see comes into play! I'm a huge fan 👏

@filmaj filmaj merged commit b840c0b into main Oct 31, 2022
@filmaj filmaj deleted the type-common-props-refactor branch October 31, 2022 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typescript issues that specifically impact using the package from typescript projects discussion M-T: An issue where more input is needed to reach a decision enhancement M-T: A feature request for new functionality pkg:types applies to `@slack/types` question M-T: User needs support to use the project semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants