-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: add pigeon #1712
feat: add pigeon #1712
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 11 of 11 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @evemartin)
a discussion (no related file):
The if cows
block is still the same, it needs to be dynamic too ideally, otherwise we can just have it say if animal
which is similar to what the Python code has anyway
game/static/game/js/animation.js
line 44 at r2 (raw file):
for (let i = 0 ; i < cows.length ; i++){ let cow = cows[i]; console.log(cow.type);
Remove
game/static/game/js/cow.js
line 26 at r2 (raw file):
}; ocargo.Cow.prototype.queueLeaveAnimation = function(model, node) {
How much time do you reckon you'd need to rename the Cow
object to Animal
? 😕 Is it used in a lot of places / would it cause a lot of issues?
Screencast from 03-09-24 10:02:11.webm |
Screencast from 03-09-24 10:10:29.webm |
Thanks @SKairinos , some of these will be separate tasks. Elements should all rotate if clicked on as some roads will be vertical and some horizontal - is that what you mean? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 9 of 19 files reviewed, 3 unresolved discussions (waiting on @faucomte97 and @SKairinos)
a discussion (no related file):
Previously, faucomte97 (Florian Aucomte) wrote…
The
if cows
block is still the same, it needs to be dynamic too ideally, otherwise we can just have it sayif animal
which is similar to what the Python code has anyway
Thank you both for the reviews, in addition to the changes I've mentioned in other comments I've also changed the cows
block to dynamically update to pigeons
! @SKairinos I've had a look at the issues you pointed out and since they apply to cows as well as pigeons I think it would make sense for them to be done in their own tasks.
game/static/game/js/cow.js
line 26 at r2 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
How much time do you reckon you'd need to rename the
Cow
object toAnimal
? 😕 Is it used in a lot of places / would it cause a lot of issues?
I think it would probably take a little while just to make sure I had changed it everywhere but renaming just the object would be more doable than renaming the object plus all the associated variables - what are your thoughts on doing one of these two things?
game/static/game/js/animation.js
line 44 at r2 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Remove
Removed!
A note about the translations: I have used google translate to add translations for the word "pigeons" for the languages we have so far, since I've heard it's pretty reliable for single words. I have also added the word "pigeons" to the translations doc so that future volunteers will know, but I haven't pinged anyone asking for them to add this word to their translation. I wanted to get your opinions about this - for the translated languages that are currently verified but need this one more word, should I
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 10 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @evemartin)
To address Stefan's comments, I have made/found the following tasks:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @evemartin)
This change is