eugeneration / HappyIslandDesigner

https://eugeneration.github.io/HappyIslandDesigner/
MIT License
835 stars 143 forks source link

feat: Typescript rewrite #46

Closed bodinsamuel closed 4 years ago

bodinsamuel commented 4 years ago

Hey,

I saw your amazing project on twitter today ! ❤️ (well it was today few hours ago) I wanted to contribute, especially for the screenshot loading but I found it a bit complicated to contribute with the codebase in this current shape.

This is a purely personal initiative, I'm not pushing anything to you. It's just a more modern way to write it, but nothing against your way of doing it ☺️

I just wanted to share this with you, I don't expect this PR to be "reviewable" as it is a complete rewrite to Typescript + npm. If you are interested to know more or even want to merge, I'll be happy to this discuss. Otherwise feel free to entirely dismiss this PR 👍

Right now, I only put 3hours of work so it's not yet complete. There is a few gotchas, especially when executing paperjs outside of their controlled environnement (e.g: some automatic functions are not there like math between point, global variables etc...) But I made it run almost entirely 🤩

Screenshot 2020-03-31 02 26 50

Running

# install the dependencies
yarn

# launch the dev server with hot reload
yarn dev

go to: http://localhost:8080/

eugeneration commented 4 years ago

wow if you could get this to completely work it would be a real life saver in terms of project organization. I keep pushing off switching to a real development environment and it only keeps becoming an increasingly daunting task to do, but it looks like you've done all of the heavy lifting here!

I've had bad times setting up yarn/npm for projects which is why I was so hesitant here

eugeneration commented 4 years ago

Also are you the one who DMed me on reddit about working on the screenshot conversion feature?

bodinsamuel commented 4 years ago

I've had bad times setting up yarn/npm for projects which is why I was so hesitant here

Maybe I can help answer your question if you have any ☺️

Also are you the one who DMed me on reddit about working on the screenshot conversion feature?

Nope, but I guess many people would love this 👍


If you are interested in merging, then I can continue to update this PR. That would make merging a bit complicated with other PRs though.

eugeneration commented 4 years ago

I would definitely merge it if it works, Right now literally any added organization to the project is better than its current state and this is a full blown rewrite that modernizes the project. The other PRs that are coming in shouldn't be too hard to redo

ExperiencersInternational commented 4 years ago

@bodinsamuel as the tool doesn't work in some browsers, could you add the browser-update script maybe? And could you add templates. I'm also currently working on an Android application for the tool and I want to merge a island name generator tool that I forked too. Oh and also we need a full screen button.

bodinsamuel commented 4 years ago

I'm in the process of solving the last few issues. I should have done it in multiple steps but got carried away :3 I hope to be done by the end of the day, hopefully 🤞

ExperiencersInternational commented 4 years ago

Also I should say we need you to add some assets like in #21

ExperiencersInternational commented 4 years ago

image This is what my repo looks like rn

ExperiencersInternational commented 4 years ago

Gonna add a link to my island name generator soon to mine.

ExperiencersInternational commented 4 years ago

I think we would work good together but problem is I've never used typescript...

eugeneration commented 4 years ago

Hey FlynnFarrow, the features you mentioned shouldn't be tacked onto this PR, they should be committed as separate PRs on top of this one. Adding too much into one commit makes history messy. Also, typescript is a superset of javascript, so if you can write javascript you can write typescript.

The browser update script also only affects index.html so it shouldn't conflict (much) with this commit.

ExperiencersInternational commented 4 years ago

Adding too much into one commit makes history messy.

Yeah I get ya @eugeneration and the script only affects that, and you don't have to put the features in the whole thing, you could link to them like you link to your Twitter.

eugeneration commented 4 years ago

@bodinsamuel btw I will be landing another major change to the site, but don't let that stop you from working on this. I can redo my work on top of this commit (I would probably merge it into a separate branch that I can later merge into master once it reaches feature parity)

bodinsamuel commented 4 years ago

@eugeneration Thanks for letting me know (and congrats on shipping this !). I got hit by a few blockers than I can not fix right now:

I'm happy that I managed to fix most of the remaining bugs this weekend, it was challenging but learn a lot. Unfortunately the remaining ones are quite important 🙇‍♂️ If you want to help, please feel free to edit the branch as you should be able to do (I think).

bodinsamuel commented 4 years ago

🤯you are a beast

eugeneration commented 4 years ago

Haha here goes nothing

bodinsamuel commented 4 years ago

❤️