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

Fixed FullScreen comparison bug #286

Merged
merged 1 commit into from
Dec 28, 2023
Merged

Conversation

s-egge
Copy link
Member

@s-egge s-egge commented Dec 27, 2023

Clicking on the "Compare in FullScreen" dropdown when a non-electricity building is selected causes various issues.

image

Comparing in FullScreen when only one non-electricity building is selected or two non-electricity buildings are selected

It gets stuck on a blank page with type errors in the console

image

Comparing in FullScreen when a non-electricity and an electricity building are selected

If the non-eletricity building is clicked first, it does the same as above, but if the electricity building is selected first, it tries to compare them but can't bring up a chart for the non-electricity building

image

Code

Moving only lines 367 - 371 above the if-statement fixed it only when the non-electricity building was selected first, but moving the updateModifier logic above fixed the issue when an electric building is selected first.

@solderq35
Copy link
Contributor

I think in the future we need to support more energy types than electricity (at least Steam, as per meeting with Ross / Brandon), but for now this is a good fix.

Copy link
Contributor

@solderq35 solderq35 left a comment

Choose a reason for hiding this comment

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

I was able to confirm on my end that comparing 2 buildings (one electric, one non-electric, or two non-electric) triggered the error message correctly. For one electric and one non-electric, tested with both selecting the electric building first and non-electric building first.

@solderq35 solderq35 merged commit 7668336 into master Dec 28, 2023
3 checks passed
@s-egge s-egge deleted the comparison-menu-fullscreen-bug branch July 1, 2024 18:43
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