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

bug(web): present electric net exchange chart for 30d+ views #7594

Merged

Conversation

consideRatio
Copy link
Contributor

@consideRatio consideRatio commented Dec 20, 2024

Issue

Fixes #7593 - the issue includes an analysis and details on why this is reasonable to consider a bug fix.

Description

The net exchange chart wasn't presented for the 30d, 12mo, and all views.

Preview

image

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"
  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@VIKTORVAV99
Copy link
Member

Thanks for the PR @consideRatio!

Just a heads up that we just entered a code freeze so even if this is merged we won't be deploying new code until January.

@tonypls do you remember why we did this in the first place?

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

web/src/utils/helpers.ts:198

  • The removal of the check for an empty 'exchange' object may lead to unintended behavior. Consider re-adding this check or ensuring that 'exchange' is never empty before calling this function.
if (Object.keys(zoneData.exchange).length === 0) {
@consideRatio consideRatio changed the title bug(web): present net exchange chart for 30d+ views bug(web): present electric net exchange chart for 30d+ views Dec 20, 2024
@consideRatio
Copy link
Contributor Author

@VIKTORVAV99 thank you for the quick response! I've updated the title of this PR and the linked issue to be about the electric net exchange chart, because the emission net exchange chart is still not showing up for another reason - now tracked by #7596.

@tonypls
Copy link
Collaborator

tonypls commented Dec 20, 2024

Thanks for the PR @consideRatio!

Just a heads up that we just entered a code freeze so even if this is merged we won't be deploying new code until January.

@tonypls do you remember why we did this in the first place?

Mostly to reduce the chance our Christmas celebrations are effected by production incidents.

This will be deployed to staging.electricitymaps.com once merged! 🚀

@VIKTORVAV99
Copy link
Member

Thanks for the PR @consideRatio!

Just a heads up that we just entered a code freeze so even if this is merged we won't be deploying new code until January.

@tonypls do you remember why we did this in the first place?

Mostly to reduce the chance our Christmas celebrations are effected by production incidents.

This will be deployed to staging.electricitymaps.com once merged! 🚀

Not the code freeze 😅

Why we added the check that blocks the net exchange graph from the daily, monthly and yearly views?

@tonypls
Copy link
Collaborator

tonypls commented Dec 22, 2024

Thanks for the PR @consideRatio!

Just a heads up that we just entered a code freeze so even if this is merged we won't be deploying new code until January.

@tonypls do you remember why we did this in the first place?

Mostly to reduce the chance our Christmas celebrations are effected by production incidents.
This will be deployed to staging.electricitymaps.com once merged! 🚀

Not the code freeze 😅

Why we added the check that blocks the net exchange graph from the daily, monthly and yearly views?

Ah 😅 . I think because we don't show aggregate exchanges it didn't feel like the ideal solution but I guess because it's a net exchange it should be fine!

Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

Tested locally and it works as expected so LGTM! 🎉

Thanks for opening the PR!

@VIKTORVAV99 VIKTORVAV99 merged commit 8d6ad84 into electricitymaps:master Dec 23, 2024
23 checks passed
@consideRatio
Copy link
Contributor Author

consideRatio commented Dec 23, 2024

Thanks for the review efforts @VIKTORVAV99 and @tonypls, excited that a small fix like this enabled me to use the aggregated net exchange view which i find very interesting!

I want politicians to value low-carbon electricity exports, and not reason "but we'll manage with one less nuclear power plant or wind park" and instead value the climate impact abroad.

A view like this can help towards such insights i think!

@VIKTORVAV99
Copy link
Member

Thanks for the review efforts @VIKTORVAV99 and @tonypls, excited that a small fix like this enabled me to use the aggregated net exchange view which i find very interesting!

I want politicians to value low-carbon electricity exports, and not reason "but we'll manage with one less nuclear power plant or wind park" and instead value the climate impact abroad.

A view like this can help towards such insights i think!

No problem at all and thanks for opening the PR!

I will look into the emissions part as well (in your other issue) in the when the code freeze is over in early Jan!

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Electric net exchange chart is only presented for 24h and 72h - not for 30d, 12mo, all
3 participants