TarVK / pixi-shadows

Adds dynamic shadows to PixiJS
https://tarvk.github.io/pixi-shadows/dist/
MIT License
53 stars 14 forks source link

Makeover for the project #10

Closed amir-arad closed 2 years ago

amir-arad commented 2 years ago
amir-arad commented 2 years ago

@TarVK this is a complete project makeover in terms of language and dev stack

amir-arad commented 2 years ago

note that: global.d.ts is where to declare the global effect of the plugin on other interfaces (in addition to internally in index.d.ts) When you make changes and want to re-publish, you can use npm run release:patch to change version of lib.

TarVK commented 2 years ago

Damn, that's some investment haha. Do you think this is ready, or are you planning to make changes still? Not sure when I will have a detailed look yet, but would be good to know I'm not looking at something that will be changed soon.

I noticed you partially switched to proper TS (which is also my preference nowadays), but I see there are some JS files left. Now that these project is actually being cleaned up, maybe we should fully commit to TS?

amir-arad commented 2 years ago

the only .js files I know of are the configurations (*.config.js) and the build artifacts.

the last 3 commits were fixes I missed before making the PR. I went over it and I think it's ready. feel free to edit as you see fit

TarVK commented 2 years ago

Oh, and I definitely want the demos/examples accessible on github pages again (I should probably also refer to them earlier in the readme so the demos are easier to find). So I am not sure how to build those currently with your new setup, I only saw how to use them for testing during development.

Btw, me thinking there were still .js files was (as you probably already figured) just a mistake on my end. I used github's changes tab to quickly see what you did, and I must have accidentally seen a .JS file that you removed, but I thought it was something you added/changed.

amir-arad commented 2 years ago

starting to address issues. I did see more things I could improve but wanted to deliver the PR as it was already packed. I will open issues as I find them, for stuff that can stand being postponed IMO.

amir-arad commented 2 years ago

In 576ad3d I addresses all items except the ones I opened issues for, and the Demos.

There is still a bug in the demos, compared to the original: the casters are not hidden by shadows. I'm looking into it. Next I will look at the demos, but other than these two I'd like to hear what you think.

amir-arad commented 2 years ago

@TarVK please review changes

amir-arad commented 2 years ago

Thanks @TarVK maybe worth releasing beforehand? as a baseline of the new format and pixi6 support.

I'm going to need 2 more features for my use case, not sure if it's not going to end up as a fork if it's not part of the use-case. will write them down soon I hope. please don't feel obliged :)

TarVK commented 2 years ago

I just tried releasing. I think there's a little problem with the package scripts however. I think it started running rollup in watch mode, instead of building: terminal

(edit) Nvm, I'm stupid. I ran npm start release:major instead of npm run release:major. I'm used to yarn, but yarn didn't work for installation since a package-lock.json was present. So from there forward I just continued with npm in this project just in case, but it has been a while 😅

TarVK commented 2 years ago

I released the version. I would appreciate if you could test whether all is in order. I currently don't have any projects set up in which I could easily test it myself.

Additionally since you put quite some work in the redesign, I was wondering whether to put a section with credits/contributions in the readme. What are your own thoughts on this?