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 shapes and font options types #650

Merged
merged 2 commits into from
Jan 28, 2020

Conversation

cronin4392
Copy link
Contributor

No description provided.

@@ -302,7 +302,6 @@ export interface ITableOptions extends PositionOptions {
colspan?: number
colW?: number | number[]
fill?: Color
fontSize?: number
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this PR was to try and prevent issues like this. ITableOptions had the type for fontSize but not for fontFamily. I am thinking of doing this type of refactor in more places in order for shared types to be consistent. Thoughts @gitbrent ?

Copy link
Owner

Choose a reason for hiding this comment

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

@cronin4392 - Perfect, this is much needed

@@ -23,6 +23,10 @@ export type ShapeFill = Color | { type: string; color: Color; alpha?: number }
export type BkgdOpts = { src?: string; path?: string; data?: string }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gitbrent would you mind explaining your thinking behind having both core-*.ts files as well as types/index.d.ts? My assumption is that a goal would be to get rid of one of these files and have types coming from one place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dug in a bit more and noticed that tsconfig.js has declarationDir set (https://github.com/gitbrent/PptxGenJS/blob/master/tsconfig.json#L4).

I think that is supposed to output a types definition file when ts compiles which would mean the types/index.d.ts would not be needed. Will look into why that file is not being created in the out directory any longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gitbrent I haven't had a chance yet to track down the delcarationDir issues. I think I could look into generating the types once in another PR.

@cronin4392 cronin4392 force-pushed the add-shapes-and-font-options-types branch 2 times, most recently from ec2fcb5 to ab4ae72 Compare January 22, 2020 16:26
@cronin4392 cronin4392 force-pushed the add-shapes-and-font-options-types branch from ab4ae72 to 4a54267 Compare January 27, 2020 14:15
@gitbrent gitbrent self-assigned this Jan 28, 2020
@gitbrent
Copy link
Owner

@cronin4392 - thanks for the patch and the research.

AFAIK, only an index.d.ts file is honored typescript for definitions, so i combine all the *.ts files into a single file and ship that.

@gitbrent gitbrent merged commit 14934e3 into gitbrent:master Jan 28, 2020
@gitbrent
Copy link
Owner

Thanks @cronin4392 !

@gitbrent gitbrent added this to the 3.1.1 milestone Jan 28, 2020
gitbrent added a commit that referenced this pull request Jan 28, 2020
gitbrent added a commit that referenced this pull request Jan 28, 2020
ericrockey pushed a commit to WeTransfer/PptxGenJS that referenced this pull request Mar 24, 2020
…ions-types

Add shapes and font options types
ericrockey pushed a commit to WeTransfer/PptxGenJS that referenced this pull request Mar 24, 2020
ericrockey pushed a commit to WeTransfer/PptxGenJS that referenced this pull request Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants