feat(web): unify unit formatting

[silkeholmebonnen]

Issue

AVO-255

Description

This PR unifies the unit formatting so that it uses the same unit when displaying a value and the total value.

Preview

Old:

New:

Old:

New:

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.

[silkeholmebonnen]

I would like to hear your thoughts on the number of digits we should use and if we want to do it this way or if you can think of a better way to do it?

@madsnedergaard, @VIKTORVAV99, @tonypls and @Alportan

[silkeholmebonnen]

In a case like this the number gets pretty long.

[VIKTORVAV99]

I would like to hear your thoughts on the number of digits we should use and if we want to do it this way or if you can think of a better way to do it?

@madsnedergaard, @VIKTORVAV99, @tonypls

Hmm, that's a lot of decimals in some places. Which might be fine information wise but have we checked that it won't break the UI on smaller devices in particular?

And I'm wondering if it won't be harder to understand for most if there is a bunch of zeros in there?

---

There is also this edge case:

If the plan is to unify all the units it doesn't make much sense if part of the tooltip is in GW and the other parts are in MW.

---

I'm also just curious why you decided that the function should take an object as the argument. Nothing against it just curious about the reasoning here 🙂

[silkeholmebonnen]

I'm also just curious why you decided that the function should take an object as the argument. Nothing against it just curious about the reasoning here 🙂

I did that so that both the total and the numberDigits parameters could be optional, so that you can specify which parameter you want to assign to which value when using it.

[silkeholmebonnen]

Hmm, that's a lot of decimals in some places. Which might be fine information wise but have we checked that it won't break the UI on smaller devices in particular?

Yes, we should check this. I haven't done that yet.

And I'm wondering if it won't be harder to understand for most if there is a bunch of zeros in there?

I guess that based on the feedback that we have gotten, the assumption is that it easier to compare two numbers if they are using the same unit, than comparing two numbers that use different units. And if there is a bunch of zeros then it is also easier to see that this number is really small compared to the total, which might be fine I think

[silkeholmebonnen]

If the plan is to unify all the units it doesn't make much sense if part of the tooltip is in GW and the other parts are in MW.

I think that is fine. I don't think the plan is to unify all the units, just to compare apples with apples, if that makes sense:)

[silkeholmebonnen]

I just found this edge case, where the installed capacity is 0 so the value will be formatted by itself, since there is nothing to compare against. But in this case do you think we should then format the total (installed capacity) against the actual value, so that it would be 849 MW / 0 MW? Or is it not that important since the total is 0? It would require some logic to make it 849 MW / 0 MW, so do you think it is worth it?

[Alportan]

But in this case do you think we should then format the total (installed capacity) against the actual value, so that it would be 849 MW / 0 MW? Or is it not that important since the total is 0? It would require some logic to make it 849 MW / 0 MW, so do you think it is worth it?

I would say that for this edge case it's not really worth it to invest more effort for custom logic.

Where I do see some value though is, in case the capacity is not reported (I assume that's why it's 0) we should somehow explain why, but this is more of a UX task and unrelated to this PR. 😅 Same thing in case the reported capacity is lower than production.

[silkeholmebonnen]

@VIKTORVAV99 and @tonypls, I just want to make sure that you did not forget about this PR 😄

[tonypls]

An edge case but in the example on Lanzarote when there's no capacity it shows as W when I would expect it to match the MW from the preceeding unit

[silkeholmebonnen]

An edge case but in the example on Lanzarote when there's no capacity it shows as W when I would expect it to match the MW from the preceeding unit

We already discussed this further up ☝️ and decided that this edge case wasn't worth the extra logic that it would require. Do you think it is worth it?

[silkeholmebonnen]

@tonypls Can I get this reviewed 🙏

[tonypls]

/review

[github-actions]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Typo in Parameter The parameter name paramerters in the format function should be corrected to parameters for clarity and to avoid potential confusion. Redundant Code The check value == undefined is redundant because Number.isNaN(value) will already handle cases where value is undefined. Consider simplifying the condition.

[tonypls]

Looks good to me! While reviewing and I had context I decided to add in the check to show a "?" if the tooltip capacity is missing. Please review my code, merge if you're happy or @ me if I need to improve it!

[silkeholmebonnen]

Your changes looked good to me. I made some small changes based on Githubs review:)