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

feat: add pigeon #1712

Merged
merged 29 commits into from
Sep 4, 2024
Merged

feat: add pigeon #1712

merged 29 commits into from
Sep 4, 2024

Conversation

evemartin
Copy link
Contributor

@evemartin evemartin commented Aug 30, 2024

This change is Reviewable

@evemartin evemartin linked an issue Aug 30, 2024 that may be closed by this pull request
4 tasks
Copy link
Contributor

@faucomte97 faucomte97 left a 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?

@SKairinos
Copy link
Contributor

Screencast from 03-09-24 10:02:11.webm
Pigeon rotates if you re-place it

@SKairinos
Copy link
Contributor

placing a pigeon on the map does not auto-insert the pigeon block into the list of available blocks. It shouldn't be possible to have a pigeon without the pigeon block.
image

@SKairinos
Copy link
Contributor

SKairinos commented Sep 3, 2024

What is the django migr button?
image
image

@SKairinos
Copy link
Contributor

Screencast from 03-09-24 10:10:29.webm
IMO, placing a pigeon on a red square should auto-delete that pigeon. Allowing it's placement in the editor communicates to me that the placement was successful.

@SKairinos
Copy link
Contributor

Still says cows in the level editor
image

@lauracumming
Copy link

lauracumming commented Sep 3, 2024

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?

Copy link
Contributor Author

@evemartin evemartin left a 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 say if 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 to Animal? 😕 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!

@evemartin
Copy link
Contributor Author

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

  1. send out a mass message in the relevant channels asking people to translate this one word
  2. message people who worked on the translations specifically to translate this one word (based on doc edit history)
  3. use Google Translate just for the word "pigeons"

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 10 of 10 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @evemartin)

@evemartin
Copy link
Contributor Author

To address Stefan's comments, I have made/found the following tasks:

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @evemartin)

@evemartin evemartin merged commit 5c7029b into master Sep 4, 2024
5 checks passed
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.

Replace cows with pigeon in City theme
4 participants