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.

Sign up for GitHub

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

Jump to bottom

Switch to TypeScript #57

Closed
wants to merge 3 commits into from
Closed

Switch to TypeScript #57

wants to merge 3 commits into from

Conversation

youknowedo
Copy link

@youknowedo youknowedo commented Aug 6, 2022 •
edited by himynameisdave

Added TypeScript and annotations for a better developer experience for those who like to have everything typed. "frappe-charts" isn't typed either so most of the types in the .d.ts are option types for that package.

Closes #57

@youknowedo youknowedo mentioned this pull request Aug 6, 2022
Open
@Peter-Barrett
Copy link

@himynameisdave it would be great if you could merge this in. Thanks.

@silvestrevivo
Copy link

Any updates about this?

@Peter-Barrett
Copy link

For the moment adding // @ts-nocheck above the import is doing the job for me but it would be great to have this merged @himynameisdave .

himynameisdave
himynameisdave approved these changes Sep 16, 2022
View reviewed changes
Copy link
Owner

@himynameisdave himynameisdave left a comment

Choose a reason for hiding this comment

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

@fenwikk thanks for writing these definitions. 🎉 💯 I have a few questions around the added dependencies, but otherwise this looks good to me!

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/components/base.svelte
export let maxSlices = 3;
export let data: Data;
export let title: string = '';
export let type: ChartType = 'line';
Copy link
Owner

Choose a reason for hiding this comment

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

[nitpick] I get that these types are ambient, but generally I prefer to import and use types directly (using type imports). Not sure if this is standard/best practice though, thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

doesnt matter to me. could change it to a non global declaration file if you want

tsconfig.json Show resolved Hide resolved
@himynameisdave himynameisdave added Type: Feature Adds a new feature Status: Changes Requested Requires changes before merging labels Sep 16, 2022
@himynameisdave
Copy link
Owner

Ping @fenwikk, let me know if there's anything I can do to help get this over the finish line.

@himynameisdave
Copy link
Owner

Ping @fenwikk, sorry to be annoying here (as I really appreciate the work you've already done here), but if you're unable to address the comments I'd be happy to wrap things up here, just let me know 🙂

@SemmelJochen
Copy link

any news on this? Can I help somehow?

@tmtygnz
Copy link

tmtygnz commented Jul 17, 2023

Any news on this?

@youknowedo
Copy link
Author

sorry guys just been busy ill fix whatever needs fixing

@youknowedo youknowedo closed this by deleting the head repository Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SemmelJochen SemmelJochen SemmelJochen left review comments

@himynameisdave himynameisdave himynameisdave approved these changes

Assignees
No one assigned
Labels
Status: Changes Requested Requires changes before merging Type: Feature Adds a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@youknowedo @Peter-Barrett @silvestrevivo @himynameisdave @SemmelJochen @tmtygnz