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

upgrade to modern angular APIs #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jabiinfante
Copy link

@jabiinfante jabiinfante commented Apr 15, 2024

Hi,
Thanks for the template.
I have found it really useful and easy to understand for angular developers willing to step into Phaser.

I have a few suggestions to make (split into two commits):

upgrade to latest + eslint

  • I have upgraded to the latest available angular version (17.3.4)
  • I have added angular eslint schematic with the default configuration
  • I have also added a consistent type imports and a couple of more rules

refactor to new angular APIs.

  • I have used signals to keep the state within the template (moveSprinte, spritePosition, etc)
  • Remove unused imports on some components
  • Added some type guards avoiding the use of " as "
  • remove @ViewChild in favor of viewChild.required

I hope it proves helpful. Nevertheless, I'm open to discussion or further suggestions.

@photonstorm
Copy link

Thanks for this PR. Can I please check - the eslint configuration: Is this something you just personally find useful, or does this version of Angular actually require it? I'm asking because we generally avoid adding opinionated lint rules in our templates, because they're meant to be used the way you want to. However, if this is something Angular itself requires, that's different.

@jabiinfante
Copy link
Author

I added ESLint using @angular-eslint/schematics.
It's not required at all by angular (allthougth recommended).

But I see your point, and makes perfect sense.
I will update the PR removing the ESLint later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants