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

Types: split single index.ts file into files based on function/category #1656

Merged
merged 4 commits into from Sep 5, 2023

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Aug 31, 2023

Summary

This is purely a text-movement PR; no changes (other than a few comments at the top of certain files) from the base.

I'm mainly looking for feedback on the organization of the files. If you have feedback on directory structure or file names, let me know!

@filmaj filmaj added semver:patch area:typescript issues that specifically impact using the package from typescript projects pkg:types applies to `@slack/types` labels Aug 31, 2023
@filmaj filmaj added this to the types@2.8.1 milestone Aug 31, 2023
@filmaj filmaj requested a review from a team August 31, 2023 21:04
@filmaj filmaj self-assigned this Aug 31, 2023
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 the structure! Left a few small suggestions most notably renaming elements.ts as block-elements.ts.

export type KnownBlock = ImageBlock | ContextBlock | ActionsBlock | DividerBlock |
SectionBlock | InputBlock | FileBlock | HeaderBlock | VideoBlock;

export interface ActionsBlock extends Block {
Copy link
Member

Choose a reason for hiding this comment

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

Seems worth it to adding additional annotation to more easily find the documentation link while we're at it?

Fine to punt to the later TODO moment referenced in the comments above as well.

Suggested change
export interface ActionsBlock extends Block {
// Reference: https://api.slack.com/reference/block-kit/blocks#actions
export interface ActionsBlock extends Block {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly, I will be doing exactly these kind of changes in the next pull request 😄 I didn't want to add more additional lines in this specific PR as the diff would be too confusing given the movement of code across files. I think that in the next PR, though, these kinds of documentation additions you suggest would be clearer and easier to review.

packages/types/src/block-kit/elements.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,38 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Nice that we'll just be able to poof this file when major version bump comes along 🧹 !

packages/types/src/views.ts Show resolved Hide resolved
packages/types/src/views.ts Show resolved Hide resolved
filmaj and others added 3 commits September 1, 2023 08:38
Co-authored-by: Sarah Jiang <sarahjiang@slack-corp.com>
Co-authored-by: Sarah Jiang <sarahjiang@slack-corp.com>
@filmaj filmaj requested a review from srajiang September 1, 2023 12:45
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.

Great, LGTM!

@filmaj filmaj merged commit 09ef142 into main Sep 5, 2023
15 checks passed
@filmaj filmaj deleted the types-split-files branch September 5, 2023 20:04
@filmaj filmaj modified the milestones: types@2.8.1, types@2.9.0 Sep 25, 2023
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 pkg:types applies to `@slack/types` semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants