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

Object Oriented JavaScript #51

Merged
merged 8 commits into from
Aug 9, 2022

Conversation

Eugene-CG
Copy link
Contributor

Object oriented javascript

Demo
Code base

The code is submitted in a dedicated feature branch.

Only code files are submitted.

Please, review.

Copy link
Collaborator

@A-Ostrovnyy A-Ostrovnyy left a comment

Choose a reason for hiding this comment

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

Awesome!

submissions/Eugene-CG/OOJS-Frogger-Game/app.js Outdated Show resolved Hide resolved
submissions/Eugene-CG/OOJS-Frogger-Game/app.js Outdated Show resolved Hide resolved
Minor change: numerical comparison now changed to comprasion with obj property
Minot change:
forEach and push methods changed to .map method
@Eugene-CG
Copy link
Contributor Author

Eugene-CG commented Aug 2, 2022

Little mistake in last commit comment -_-
Don`t know how to change this using github, only git commit --amend -m "..." =|

Copy link
Contributor Author

@Eugene-CG Eugene-CG left a comment

Choose a reason for hiding this comment

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

Great Jobe Myself Забавна штука, я теж так можу, хе :3
а як видалити =Ґ

@Eugene-CG Eugene-CG closed this Aug 2, 2022
@A-Ostrovnyy
Copy link
Collaborator

@Eugene-CG why you close this PR?

@Eugene-CG Eugene-CG reopened this Aug 2, 2022
@Eugene-CG
Copy link
Contributor Author

Eugene-CG commented Aug 2, 2022

@Eugene-CG why you close this PR?

Sorry, I want to delete my comment and click everything I see
😢

@A-Ostrovnyy
Copy link
Collaborator

To avoid this situation I always use my code editor instead of web interface) don't worry about the same commit message, if you want to change it use --amend

@Eugene-CG
Copy link
Contributor Author

To avoid this situation I always use my code editor instead of web interface) don't worry about the same commit message, if you want to change it use --amend

Already changed :3

Copy link
Member

@OleksiyRudenko OleksiyRudenko left a comment

Choose a reason for hiding this comment

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

@Eugene-CG great job!
A couple of improvements and we are done.

submissions/Eugene-CG/OOJS-Frogger-Game/app.js Outdated Show resolved Hide resolved
Comment on lines 59 to 65
if (this.y < tileSize.height - charactersSize.charboy) {
setTimeout(() => {
alert("You won");
this.y = tileSize.height * 4 - charactersSize.charboy;
this.x = tileSize.width * 2;
}, 100);
}
Copy link
Member

Choose a reason for hiding this comment

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

The method name is imperative, however the body assumes a condition. This confuses me as a peer developer.
You may want either rename the method or do conditional checks elsewhere where coordinates change happens and let this method just set some coordinates imperatively.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still not fixed 😢

@OleksiyRudenko OleksiyRudenko self-assigned this Aug 7, 2022
@Eugene-CG Eugene-CG closed this Aug 7, 2022
Copy link
Collaborator

@A-Ostrovnyy A-Ostrovnyy left a comment

Choose a reason for hiding this comment

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

One last issue)

submissions/Eugene-CG/OOJS-Frogger-Game/app.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@A-Ostrovnyy A-Ostrovnyy left a comment

Choose a reason for hiding this comment

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

Well done!

@A-Ostrovnyy A-Ostrovnyy merged commit c05ae9d into kottans:main Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants