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(web): add future price #7227

Conversation

silkeholmebonnen
Copy link
Contributor

@silkeholmebonnen silkeholmebonnen commented Sep 23, 2024

Issue

AVO-455

Description

This PR adds a section to the price card that shows future prices for the zones that we have future price for (nordpool zones).

Preview

image

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

package.json Outdated Show resolved Hide resolved
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Sep 23, 2024
@silkeholmebonnen
Copy link
Contributor Author

Feel free to take a look at the future price @tonypls @Alportan @cadeban 🌞

@silkeholmebonnen
Copy link
Contributor Author

I'll just do a few changes @VIKTORVAV99. I found a bug that I will fix and we decided in the office that we will try and switch to MWh.

@VIKTORVAV99
Copy link
Member

I'll just do a few changes @VIKTORVAV99. I found a bug that I will fix and we decided in the office that we will try and switch to MWh.

Sounds good, I'll probably not have time to take a deep look at this until tomorrow or Wednesday. 🙂

@VIKTORVAV99
Copy link
Member

I'll just do a few changes @VIKTORVAV99. I found a bug that I will fix and we decided in the office that we will try and switch to MWh.

@Alportan there is also a issue open here #7219 to show prices in Euro Cent per KWh, which might be an option. But I agree on doing MWh now and then we can look into cents later on.

@Alportan
Copy link
Contributor

I'll just do a few changes @VIKTORVAV99. I found a bug that I will fix and we decided in the office that we will try and switch to MWh.

@Alportan there is also a issue open here #7219 to show prices in Euro Cent per KWh, which might be an option. But I agree on doing MWh now and then we can look into cents later on.

Nice! 🙌 Let's start with switching back to MWh for now.

It's sneaky, but this way we can monitor incoming feedback the other way (people being upset about switching back to MWh instead of kWh) then we have some more evidence for the need of a toggle between the 2 views. 🥷

@silkeholmebonnen
Copy link
Contributor Author

silkeholmebonnen commented Sep 23, 2024

It should be ready for review now. The only thing missing is the highligt of the current hour (as seen here), but that could also be done in v2. I will not have time to implement this until Thursday, so you are welcome to do it @VIKTORVAV99, or I can do it Thursday, or we can wait until v2 - whichever you prefer.

image

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.

Looks really good!

Some minor comments but overall it's a pretty clean implementation!

web/src/features/charts/FuturePrice.tsx Outdated Show resolved Hide resolved
web/src/features/charts/futurePriceUtils.ts Outdated Show resolved Hide resolved
web/src/features/charts/futurePriceUtils.ts Outdated Show resolved Hide resolved
web/src/utils/state/atoms.ts Outdated Show resolved Hide resolved
web/src/features/charts/FuturePrice.tsx Outdated Show resolved Hide resolved
web/src/features/charts/FuturePrice.tsx Outdated Show resolved Hide resolved
@VIKTORVAV99
Copy link
Member

I did notice a blocker for this but it's related to the backend, basically we need to double check that we have a license for the zone before we send the day ahead data. If you look at Croatia for example then we hide the data in the graph due to license issues but you can still see the day ahead prices.

silkeholmebonnen and others added 4 commits September 26, 2024 09:08
Co-authored-by: Viktor Andersson <30777521+VIKTORVAV99@users.noreply.github.com>
Co-authored-by: Viktor Andersson <30777521+VIKTORVAV99@users.noreply.github.com>
@silkeholmebonnen
Copy link
Contributor Author

I did notice a blocker for this but it's related to the backend, basically we need to double check that we have a license for the zone before we send the day ahead data. If you look at Croatia for example then we hide the data in the graph due to license issues but you can still see the day ahead prices.

Until we change it in the backend we would be able to do a simple check like this in the frontend to ensure that we only show future price if the price is not disabled:
{!isPriceDisabled && <FuturePrice futurePrice={futurePrice} />}

Do you think we should do that for now?

@VIKTORVAV99
Copy link
Member

I did notice a blocker for this but it's related to the backend, basically we need to double check that we have a license for the zone before we send the day ahead data. If you look at Croatia for example then we hide the data in the graph due to license issues but you can still see the day ahead prices.

Until we change it in the backend we would be able to do a simple check like this in the frontend to ensure that we only show future price if the price is not disabled:

{!isPriceDisabled && <FuturePrice futurePrice={futurePrice} />}

Do you think we should do that for now?

Not sure that's good enough, we would still send the data. But it shouldn't be too hard to do it in the backend as we are already doing it for the other price information.

@silkeholmebonnen
Copy link
Contributor Author

I did notice a blocker for this but it's related to the backend, basically we need to double check that we have a license for the zone before we send the day ahead data. If you look at Croatia for example then we hide the data in the graph due to license issues but you can still see the day ahead prices.

Until we change it in the backend we would be able to do a simple check like this in the frontend to ensure that we only show future price if the price is not disabled:
{!isPriceDisabled && <FuturePrice futurePrice={futurePrice} />}
Do you think we should do that for now?

Not sure that's good enough, we would still send the data. But it shouldn't be too hard to do it in the backend as we are already doing it for the other price information.

Okay, I will change it in the backend ASAP:)

@Alportan
Copy link
Contributor

Not sure that's good enough, we would still send the data. But it shouldn't be too hard to do it in the backend as we are already doing it for the other price information.

How long do you estimate it would take to implement the backend changes for checking the license before sending day-ahead data? 🙋

Given that we're getting more user feedback requesting prices in MWh instead of kWh (and now in the form of appstore reviews), we could consider cherry-picking just that change and release it separately if the full set of changes wouldn't be as straightforward. 💡

Alternatively, if the backend changes are quick, we should proceed with the full implementation. 💪

What do you think the best approach would be? 🙌 @VIKTORVAV99 @silkeholmebonnen

@VIKTORVAV99
Copy link
Member

Not sure that's good enough, we would still send the data. But it shouldn't be too hard to do it in the backend as we are already doing it for the other price information.

How long do you estimate it would take to implement the backend changes for checking the license before sending day-ahead data? 🙋

Given that we're getting more user feedback requesting prices in MWh instead of kWh (and now in the form of appstore reviews), we could consider cherry-picking just that change and release it separately if the full set of changes wouldn't be as straightforward. 💡

Alternatively, if the backend changes are quick, we should proceed with the full implementation. 💪

What do you think the best approach would be? 🙌 @VIKTORVAV99 @silkeholmebonnen

They just got merged 😉

@Alportan
Copy link
Contributor

Not sure that's good enough, we would still send the data. But it shouldn't be too hard to do it in the backend as we are already doing it for the other price information.

How long do you estimate it would take to implement the backend changes for checking the license before sending day-ahead data? 🙋
Given that we're getting more user feedback requesting prices in MWh instead of kWh (and now in the form of appstore reviews), we could consider cherry-picking just that change and release it separately if the full set of changes wouldn't be as straightforward. 💡
Alternatively, if the backend changes are quick, we should proceed with the full implementation. 💪
What do you think the best approach would be? 🙌 @VIKTORVAV99 @silkeholmebonnen

They just got merged 😉

Then maybe we can soon release this soon 🤩 👏 Let's prioritise getting it out there! 😎

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.

LGTM! 🎉

I look forward to spending hours and hours looking at prices!

Nice work!

@silkeholmebonnen silkeholmebonnen enabled auto-merge (squash) September 26, 2024 08:34
@silkeholmebonnen silkeholmebonnen merged commit 673bbbe into master Sep 26, 2024
22 checks passed
@silkeholmebonnen silkeholmebonnen deleted the silkebonnen/avo-455-add-upcoming-price-section-for-nordpool-areas branch September 26, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants