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

Add Timepicker to @slack/types #1226

Merged
merged 2 commits into from May 6, 2021
Merged

Add Timepicker to @slack/types #1226

merged 2 commits into from May 6, 2021

Conversation

raycharius
Copy link
Contributor

@raycharius raycharius commented May 6, 2021

Summary

Noticed that @slack/types does not include an interface for the time picker block element. Added it and included it in all of the corresponding union types.

Also, I think there are some opportunities for refactoring that would make this package even more useful than it already is. While developing v.2 of slack-block-builder, I've noticed I'm doing a lot of typing on my end, since a lot of the types aren't available through this package.

  • Moving union types to their own exported types. For example, elements supported within the ActionsBlock having a type of ActionsElements. For SectionBlock, SectionElements. Etc.
  • Moving enum values to their own exported enums.

Happy to take a shot at that, too, and send over a PR, if that's something you could see in the scope here.

Cheers!

Requirements (place an x in each [ ])

@gitwave gitwave bot added the untriaged label May 6, 2021
@seratch seratch added enhancement M-T: A feature request for new functionality pkg:types applies to `@slack/types` semver:minor and removed untriaged labels May 6, 2021
@seratch seratch added this to the types@2.x milestone May 6, 2021
@seratch
Copy link
Member

seratch commented May 6, 2021

Hi @raycharius, thanks for taking the time to make this PR! The current revision seems to have a lint error. Can you fix the error?

> @slack/types@2.0.0 lint /home/runner/work/node-slack-sdk/node-slack-sdk/packages/types
> tslint --project .

ERROR: /home/runner/work/node-slack-sdk/node-slack-sdk/packages/types/src/index.ts:295:1 - Exceeds maximum line length of 120

lerna ERR! npm run lint stderr:

Apart from that, the changes look good to me. For the timing to merge this, I have been holding off adding timepicker in the Python SDK as it's still in a developer beta. See slackapi/python-slack-sdk#876 for the context. However, the beta has been taking longer than I expect and even now, I cannot tell when the GA release will come yet. Therefore, I'm leaning toward merging the changes. If the CI failure is fixed, we are happy to merge the change.

Also, thanks for bringing the further suggestions! As I would like to make this PR's scope as small as possible, I will create a new issue for discussing other improvements. Let's discuss them separately!

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

the CI build is failing

@raycharius
Copy link
Contributor Author

Oops! Fixed. It's a bit ugly and hard to read with that union type on each line in the actual interface, but easily fixed if proposals for moving union types to their own exported type

@seratch seratch modified the milestones: types@2.x, types@2.1 May 6, 2021
@stevengill stevengill merged commit a37c8cd into slackapi:main May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality pkg:types applies to `@slack/types` semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants