Skip to content

Add Timepicker to @slack/types#1226

Merged
stevengill merged 2 commits intoslackapi:mainfrom
raycharius:add-time-picker
May 6, 2021
Merged

Add Timepicker to @slack/types#1226
stevengill merged 2 commits intoslackapi:mainfrom
raycharius:add-time-picker

Conversation

@raycharius
Copy link
Contributor

@raycharius raycharius commented May 6, 2021 •
edited
Loading

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 [email protected] milestone May 6, 2021
@seratch
Copy link
Contributor

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/[email protected] 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!

seratch
seratch suggested changes May 6, 2021
View reviewed changes
Copy link
Contributor

@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

@seratch seratch mentioned this pull request May 6, 2021
Closed
9 tasks
@raycharius
Copy link
Contributor Author

raycharius commented May 6, 2021

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

@stevengill stevengill merged commit a37c8cd into slackapi:main May 6, 2021
@seratch seratch mentioned this pull request May 6, 2021
Merged
11 tasks
This was referenced Jun 7, 2021
Closed
Merged
@filmaj filmaj mentioned this pull request Jun 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevengill stevengill stevengill approved these changes

+1 more reviewer

@seratch seratch seratch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement M-T: A feature request for new functionality pkg:types applies to `@slack/types` semver:minor

Projects

None yet

Milestone

[email protected]

Development

Successfully merging this pull request may close these issues.

3 participants

@raycharius @seratch @stevengill