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 electric van #1649

Merged
merged 2 commits into from
Jun 18, 2024
Merged

feat: add electric van #1649

merged 2 commits into from
Jun 18, 2024

Conversation

evemartin
Copy link
Contributor

@evemartin evemartin commented Jun 11, 2024

This change is Reviewable

Copy link
Contributor

@SKairinos SKairinos left a 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 r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @evemartin)


game/migrations/0093_alter_level_character_name.py line 13 at r1 (raw file):

    operations = [
        migrations.AlterField(

why can't i see the model's field being updated in the PR?


holding_assets/origial_images/electric_van.svg line 0 at r1 (raw file):
why do we have 3 different images of the van. in which case are each used?

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: all files reviewed, 2 unresolved discussions (waiting on @SKairinos)


game/migrations/0093_alter_level_character_name.py line 13 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

why can't i see the model's field being updated in the PR?

The model's field doesn't contain an explicit list of options, the choices are defined by a function that gets all the possible character choices - my changes in character.py where I added the electric van to the list of characters are what prompted this migration!


holding_assets/origial_images/electric_van.svg line at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

why do we have 3 different images of the van. in which case are each used?

Front view - used on the start block and in the level editor's character tab
Top view - used for the character that moves on the map while playing a level
I'm not sure the other one (i.e. without front or top view) is actually used anywhere, but I'm a little nervous to get rid of it because I'm worried that I've overlooked something since all the other characters have corresponding images in the same places

Copy link
Contributor

@SKairinos SKairinos 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @evemartin)


holding_assets/origial_images/electric_van.svg line at r1 (raw file):

Previously, evemartin wrote…

Front view - used on the start block and in the level editor's character tab
Top view - used for the character that moves on the map while playing a level
I'm not sure the other one (i.e. without front or top view) is actually used anywhere, but I'm a little nervous to get rid of it because I'm worried that I've overlooked something since all the other characters have corresponding images in the same places

Let's leave it for now then. Will address in the refactor.

@evemartin evemartin merged commit d0fe4c3 into master Jun 18, 2024
6 checks passed
@evemartin evemartin deleted the add-electric-van branch June 18, 2024 10:29
evemartin added a commit that referenced this pull request Jun 24, 2024
* add electric van

* add migration to include electric van
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.

2 participants