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 solar panels #1647

Merged
merged 2 commits into from
Jun 18, 2024
Merged

feat: add solar panels #1647

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, 3 unresolved discussions (waiting on @evemartin)


game/decor.py line 352 at r1 (raw file):

        name="solar_panel",
        url="decor/grass/solar_panel.svg",
        xmas_url="decor/snow/tree1.svg",

please add a ticket to the backlog to create the snow equivalent of these assets.


game/end_to_end_tests/test_level_editor.py line 143 at r1 (raw file):

        Select(self.selenium.find_element(By.ID, "theme_select")).select_by_value("snow")
        solar_panel_snow_style = self.selenium.find_element(By.ID, "solar_panel").get_attribute("style")
        assert "none" in solar_panel_snow_style

don't understand what this testing. add a docstring to the test method and maybe comments to explain what is being tested


game/static/game/js/level_editor.js line 2364 at r1 (raw file):

                    element.style = "display: inline;"
                } else {
                    element.style = "display: none;"

rather than hiding the solar panels, can we just not add them DOM tree? or would that require too much refactoring? I understand RR is chaotic so let me know if too much effort. I'd rather we take the lazy solution.

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 10 files reviewed, 3 unresolved discussions (waiting on @SKairinos)


game/decor.py line 352 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

please add a ticket to the backlog to create the snow equivalent of these assets.

Added to the Design and Research board!


game/end_to_end_tests/test_level_editor.py line 143 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

don't understand what this testing. add a docstring to the test method and maybe comments to explain what is being tested

Docstring added!


game/static/game/js/level_editor.js line 2364 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

rather than hiding the solar panels, can we just not add them DOM tree? or would that require too much refactoring? I understand RR is chaotic so let me know if too much effort. I'd rather we take the lazy solution.

I've had a look at this and I think it is much more complicated to not add/readd the element to the DOM tree rather than just hiding it - for now I think it would be best to leave it, especially since we'll be adding other graphics for the solar panels anyway, so we might not have to continue hiding them long-term

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

@evemartin evemartin merged commit 37a80a5 into master Jun 18, 2024
5 checks passed
@evemartin evemartin deleted the add-solar-panels branch June 18, 2024 12:47
evemartin added a commit that referenced this pull request Jun 24, 2024
* add solar panels

* address PR comments
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