fix(web): day ahead price styling features v3 #7274

silkeholmebonnen · 2024-10-03T14:12:13Z

Issue

Description

This PR highlights the top/bottom 5 percentage as green / red instead of just one value. It also shows 0 as a circle (12x*12 px) instead of nothing.

This is a follow up PR to #7272, so lets get that merged first.

Preview

Before:

After:

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.

…yling-features

Alportan · 2024-10-07T12:57:56Z

Quick feedback that @VIKTORVAV99 brought up 💡

Maybe we can ditch the At completely for a cleaner view. Looking at it again, it might not necessarily bring more value, so we can ditch it altogether and go for a cleaner (and more small device friendly) option. ✨ What do you think @silkeholmebonnen ? 🙌

silkeholmebonnen · 2024-10-07T14:41:20Z

Quick feedback that @VIKTORVAV99 brought up 💡

Maybe we can ditch the At completely for a cleaner view. Looking at it again, it might not necessarily bring more value, so we can ditch it altogether and go for a cleaner (and more small device friendly) option. ✨ What do you think @silkeholmebonnen ? 🙌

Sounds good! I tried to play around with how we can avoid the space being so big when it is in languages that doesn't have AM/PM or similar, but I didn't find a good solution. Do you have an idea @VIKTORVAV99?

VIKTORVAV99 · 2024-10-07T15:09:40Z

Quick feedback that @VIKTORVAV99 brought up 💡

Maybe we can ditch the At completely for a cleaner view. Looking at it again, it might not necessarily bring more value, so we can ditch it altogether and go for a cleaner (and more small device friendly) option. ✨ What do you think @silkeholmebonnen ? 🙌

Sounds good! I tried to play around with how we can avoid the space being so big when it is in languages that doesn't have AM/PM or similar, but I didn't find a good solution. Do you have an idea @VIKTORVAV99?

I don't think the space is an issue but if we think it is I'd either pad it to center the time a bit more or give more space to the actual graph.

But like I said I don't think it's an issue.

Alportan · 2024-10-08T06:56:10Z

Quick feedback that @VIKTORVAV99 brought up 💡

Maybe we can ditch the At completely for a cleaner view. Looking at it again, it might not necessarily bring more value, so we can ditch it altogether and go for a cleaner (and more small device friendly) option. ✨ What do you think @silkeholmebonnen ? 🙌

Sounds good! I tried to play around with how we can avoid the space being so big when it is in languages that doesn't have AM/PM or similar, but I didn't find a good solution. Do you have an idea @VIKTORVAV99?

I don't think the space is an issue but if we think it is I'd either pad it to center the time a bit more or give more space to the actual graph.

But like I said I don't think it's an issue.

Definitely not a blocker, but would be nice to have a more fluid layout when it comes to localising.

silkeholmebonnen · 2024-10-09T09:41:49Z

Quick feedback that @VIKTORVAV99 brought up 💡

Maybe we can ditch the At completely for a cleaner view. Looking at it again, it might not necessarily bring more value, so we can ditch it altogether and go for a cleaner (and more small device friendly) option. ✨ What do you think @silkeholmebonnen ? 🙌

Sounds good! I tried to play around with how we can avoid the space being so big when it is in languages that doesn't have AM/PM or similar, but I didn't find a good solution. Do you have an idea @VIKTORVAV99?

I don't think the space is an issue but if we think it is I'd either pad it to center the time a bit more or give more space to the actual graph.
But like I said I don't think it's an issue.

Definitely not a blocker, but would be nice to have a more fluid layout when it comes to localising.

Maybe we should leave it for now and then look at the more fluid layout later if we think it would be worth it.

VIKTORVAV99

Some comments but looks like it's working!

VIKTORVAV99 · 2024-10-09T09:46:01Z

web/src/features/charts/FuturePrice.tsx

+  useEffect(() => {
+    const handleResize = () => {
+      const divWidth = priceBarContainerReference?.current?.getBoundingClientRect().width;
+      setPositiveWidth(calculateWidth(divWidth, negativePercentage, false));
+      setNegativeWidth(calculateWidth(divWidth, negativePercentage, true));
+    };
+
+    const observer = new ResizeObserver(handleResize);
+    const current = priceBarContainerReference.current;
+
+    if (current) {
+      observer.observe(current);
+    }
+
+    return () => {
+      if (current) {
+        observer.unobserve(current);
+      }
+    };
+  }, [priceBarContainerReference, negativePercentage, isCollapsed]);


Do we really need a use effect?

It should only be used as a escape hatch if it can't fit in the normal rendering flow of react.

Wouldn't the useResizeObserver hook be a better option?

I tried just using the useResizeObserver without the useEffect but I could not get it to work. If you have time and think it is important then it would be nice if you could try it. But you decide

I can take a quick look but otherwise I don't consider this a blocking thing.

web/src/features/charts/FuturePrice.tsx

web/src/features/charts/futurePriceUtils.ts

VIKTORVAV99 · 2024-10-10T14:48:03Z

/review

github-actions · 2024-10-10T14:48:52Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Code Complexity The `FuturePrice` component has significantly increased in complexity due to new state management and dynamic styling logic. Consider refactoring to simplify and improve maintainability. Performance Concern The use of `ResizeObserver` within the `FuturePrice` component could lead to performance issues if not properly debounced or throttled, especially on lower-end devices.

silkeholmebonnen · 2024-10-14T19:46:52Z

@VIKTORVAV99, do you think I should try to refactor like GA-reviews says?

VIKTORVAV99 · 2024-10-14T19:53:37Z

@VIKTORVAV99, do you think I should try to refactor like GA-reviews says?

No I don't think we need that, I would open a backlog issue about looking into using the resizeObserver hook though. But I don't think it's blocking.

I'll give this a final review tomorrow but I don't anticipate any issues🙂

VIKTORVAV99

LGTM! 🎉

I have one minor comment though, and I think we should look into the resize observer again at some point but none of them are blocking!

VIKTORVAV99 · 2024-10-15T12:49:41Z

web/src/features/charts/FuturePrice.tsx

-                            data-test-id="negative-price"
-                          >
-                            {price < 0 && (
+                    <div>


I don't think this div is needed?

silkeholmebonnen · 2024-10-16T13:24:49Z

I just noticed this:

This was what happened when I tried using the useResizeObserver, and I thought it was only when I switched to useResizeOberser, but apparently it does it now as well... To recreate open the futureprice and adjust the width of the browser. I will look into fixing this before we merge this PR

VIKTORVAV99 · 2024-10-16T13:28:10Z

I just noticed this:

This was what happened when I tried using the useResizeObserver, and I thought it was only when I switched to useResizeOberser, but apparently it does it now as well... To recreate open the futureprice and adjust the width of the browser. I will look into fixing this before we merge this PR

I didn't notice that when using it normally (including resizing), but if you want to take another look at it don't let me stop you!

silkeholmebonnen · 2024-10-16T15:13:07Z

I think I found a much simpler way of calculating the width that fixes all the problems (not using a useEffect and the resizing problem) @VIKTORVAV99

web/src/features/charts/FuturePrice.tsx

…ub.com/electricitymaps/electricitymaps-contrib into silkebonnen/avo-559-v3-day-ahead-price

silkeholmebonnen · 2024-10-16T15:34:26Z

Do you want to give it a final review or do you think its good to go now?

VIKTORVAV99 · 2024-10-16T15:44:44Z

Do you want to give it a final review or do you think its good to go now?

I took a quick look at the last changes and I don't think it needs a new review, so just go ahead and merge it!

silkeholmebonnen and others added 13 commits September 30, 2024 14:39

fix: check if the future price data is actually future price

d7557bc

fix: now in bold

d5d8faa

WIP

6b16c83

fix: highlight of current hour

b01f3a7

fix: currecy in front for all other than €

bd793c1

fix: change order of if statements so that past hours are gray

4252417

Merge branch 'master' into silkebonnen/avo-553-v2-day-a-head-price-st…

fc19b6b

…yling-features

fix: use Intl numberformat to format currency

40f1d2e

merge master

5913bda

WIP

f511105

fix: use last index because first can be negative

8f0bf01

feat: 0 is 12 pix

3a4fbf3

feat: green and red bars for 5th percentile

b70eb91

github-actions bot added the frontend 🎨 label Oct 3, 2024

github-actions bot deployed to electricitymaps-staging (Preview) October 3, 2024 14:14 View deployment

silkeholmebonnen requested a review from Alportan October 3, 2024 14:16

remove at before time

8be1ba3

github-actions bot deployed to electricitymaps-staging (Preview) October 7, 2024 14:42 View deployment

silkeholmebonnen marked this pull request as ready for review October 9, 2024 09:41

silkeholmebonnen requested review from VIKTORVAV99 and removed request for VIKTORVAV99 October 9, 2024 09:42

silkeholmebonnen marked this pull request as draft October 9, 2024 09:46

merge master

c6d89ea

VIKTORVAV99 reviewed Oct 9, 2024

View reviewed changes

silkeholmebonnen marked this pull request as ready for review October 9, 2024 09:59

github-actions bot deployed to electricitymaps-staging (Preview) October 9, 2024 10:00 View deployment

extract reused numbers to constants

d5155eb

github-actions bot deployed to electricitymaps-staging (Preview) October 10, 2024 11:07 View deployment

silkeholmebonnen added 2 commits October 10, 2024 13:11

add minWidth 17 to tailwind config

f505ee3

fix: minwidth 18 instead of 17

fbb634e

github-actions bot deployed to electricitymaps-staging (Preview) October 10, 2024 11:14 View deployment

github-actions bot deployed to electricitymaps-staging (Preview) October 10, 2024 11:15 View deployment

use bg-success-dark

cfa1669

github-actions bot deployed to electricitymaps-staging (Preview) October 10, 2024 11:19 View deployment

github-actions bot added the Review effort [1-5]: 3 label Oct 10, 2024

VIKTORVAV99 approved these changes Oct 15, 2024

View reviewed changes

web/src/features/charts/FuturePrice.tsx

data-test-id="negative-price"

>

{price < 0 && (

<div>

Copy link

Member

VIKTORVAV99 Oct 15, 2024

Choose a reason for hiding this comment

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

I don't think this div is needed?

fix: much simpler way of calculating the width

fe29183

Merge branch 'master' into silkebonnen/avo-559-v3-day-ahead-price

e329814

github-actions bot deployed to electricitymaps-staging (Preview) October 16, 2024 15:14 View deployment

github-actions bot deployed to electricitymaps-staging (Preview) October 16, 2024 15:16 View deployment

VIKTORVAV99 reviewed Oct 16, 2024

View reviewed changes

web/src/features/charts/FuturePrice.tsx Outdated Show resolved Hide resolved

web/src/features/charts/FuturePrice.tsx Outdated Show resolved Hide resolved

silkeholmebonnen added 2 commits October 16, 2024 11:32

fix: use one calc

7a37ba0

Merge branch 'silkebonnen/avo-559-v3-day-ahead-price' of https://gith…

e05242d

…ub.com/electricitymaps/electricitymaps-contrib into silkebonnen/avo-559-v3-day-ahead-price

github-actions bot deployed to electricitymaps-staging (Preview) October 16, 2024 15:36 View deployment

silkeholmebonnen merged commit 5b7e193 into master Oct 16, 2024
22 checks passed

silkeholmebonnen deleted the silkebonnen/avo-559-v3-day-ahead-price branch October 16, 2024 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(web): day ahead price styling features v3 #7274

fix(web): day ahead price styling features v3 #7274

silkeholmebonnen commented Oct 3, 2024 •

edited

Loading

Alportan commented Oct 7, 2024

silkeholmebonnen commented Oct 7, 2024

VIKTORVAV99 commented Oct 7, 2024

Alportan commented Oct 8, 2024

silkeholmebonnen commented Oct 9, 2024

VIKTORVAV99 left a comment

VIKTORVAV99 Oct 9, 2024

silkeholmebonnen Oct 10, 2024

VIKTORVAV99 Oct 10, 2024

VIKTORVAV99 commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

silkeholmebonnen commented Oct 14, 2024

VIKTORVAV99 commented Oct 14, 2024

VIKTORVAV99 left a comment

VIKTORVAV99 Oct 15, 2024

silkeholmebonnen commented Oct 16, 2024

VIKTORVAV99 commented Oct 16, 2024 •

edited

Loading

silkeholmebonnen commented Oct 16, 2024

silkeholmebonnen commented Oct 16, 2024

VIKTORVAV99 commented Oct 16, 2024

fix(web): day ahead price styling features v3 #7274

fix(web): day ahead price styling features v3 #7274

Conversation

silkeholmebonnen commented Oct 3, 2024 • edited Loading

Issue

Description

Preview

Double check

Alportan commented Oct 7, 2024

silkeholmebonnen commented Oct 7, 2024

VIKTORVAV99 commented Oct 7, 2024

Alportan commented Oct 8, 2024

silkeholmebonnen commented Oct 9, 2024

VIKTORVAV99 left a comment

Choose a reason for hiding this comment

VIKTORVAV99 Oct 9, 2024

Choose a reason for hiding this comment

silkeholmebonnen Oct 10, 2024

Choose a reason for hiding this comment

VIKTORVAV99 Oct 10, 2024

Choose a reason for hiding this comment

VIKTORVAV99 commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

PR Reviewer Guide 🔍

silkeholmebonnen commented Oct 14, 2024

VIKTORVAV99 commented Oct 14, 2024

VIKTORVAV99 left a comment

Choose a reason for hiding this comment

VIKTORVAV99 Oct 15, 2024

Choose a reason for hiding this comment

silkeholmebonnen commented Oct 16, 2024

VIKTORVAV99 commented Oct 16, 2024 • edited Loading

silkeholmebonnen commented Oct 16, 2024

silkeholmebonnen commented Oct 16, 2024

VIKTORVAV99 commented Oct 16, 2024

silkeholmebonnen commented Oct 3, 2024 •

edited

Loading

VIKTORVAV99 commented Oct 16, 2024 •

edited

Loading