search

cyntler / hamburger-react

Animated hamburger menu icons for React.js weighs only 1.5 KB.
https://hamburger-react.netlify.app
MIT License
965 stars 36 forks source link

Rewrite package using TypeScript #10

Closed SeinopSys closed 4 years ago

SeinopSys commented 4 years ago

Companion PR to issue #9

I will say up front that I'm not sure how best to publish this to npm, but I figured this is a good starting point for it. The build command creates something that looks publishable to me but a second look would not hurt.

SeinopSys commented 4 years ago

Oh.

I'll get the docs folder TypeScript-ified then

SeinopSys commented 4 years ago

Update: This definitely does not seem to work, I tried symlinking the package folder to an existing project and importing fails. I'll keep looking into it, not sure how to mark this PR as WIP but don't merge it yet.

luukdv commented 4 years ago

Thanks so much for all your work on this. Like indicated in the related issue, TS is not really familiar territory for me. To be honest, I've read https://medium.com/javascript-scene/the-typescript-tax-132ff4cb175b at one point and haven't looked a lot at it since. I can see benefits though, but it might take a little while to get myself up to speed and grasp everything before this can be merged. Hope that's okay!

luukdv commented 4 years ago

I'll make sure not to merge it yet. At the moment it's not possible to mark PRs as drafts after they're published, but I've prepended [WIP] to the title as an indicator.

SeinopSys commented 4 years ago

Alright, I might have been using an incorrect way to test it and it was also definitely broken, so the current version was tested via running the build, then installing the folder into another project and deleting the node_modules in the dependency's folder under said project, that way I was able to import it, types resolved and the project it was imported into also managed to build. I'm hopeful this should work as is.

Maybe it wouldn't hurt to only publish this to npm under a development version tag at first to see if it actually is installable once this gets merged, this is the first time I've ever tried to make a package npm publishing compatible.

SeinopSys commented 4 years ago

I tried to follow the changes in master but the generated output does not really look all that different between the two types of bundles, so I'm worried something might be amiss.

luukdv commented 4 years ago

Looking into it! I've also simplified some stuff. It's not done yet, still need to take a look at the docs folder.