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 multiple house delivery #1611

Merged
merged 35 commits into from
May 16, 2024
Merged

Conversation

evemartin
Copy link
Contributor

@evemartin evemartin commented May 10, 2024

Add the ability to add multiple houses for delivery in a created level, as well as the ability to delete houses.


This change is Reviewable

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 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @evemartin)


game/end_to_end_tests/test_level_editor.py line 28 at r2 (raw file):

        add_road_button.click()

Remove 2 of those empty lines please


game/end_to_end_tests/test_level_editor.py line 48 at r2 (raw file):

        houses_after_delete = self.selenium.find_elements(By.CSS_SELECTOR, "rect[fill='#0000ff']")
        assert len(houses_after_delete) == 1

Could you also add a check, maybe, that when the level gets created it has the expected number of houses?
Ie from the backend Level object. Might need to be a different test though. Not 100% sure how feasible it is, if too difficult we can dismiss it.


game/static/game/js/level_editor.js line 60 at r2 (raw file):

            id: 'delete_road',
        },
        MARK_DESTINATION_MODE: {

This mode is no longer needed if I understand correctly.
Please remove all references to it, including any leftover logic surrounding it (that you haven't updated for this task) as well as the SVG it uses.

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 7 of 7 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @evemartin)

@faucomte97 faucomte97 merged commit 8ff1ad7 into master May 16, 2024
5 checks passed
@faucomte97 faucomte97 deleted the add-multiple-house-delivery branch May 16, 2024 10:41
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