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 tooltip to production source legends in BreakdownChart #6970

Conversation

silkeholmebonnen
Copy link
Contributor

@silkeholmebonnen silkeholmebonnen commented Jul 9, 2024

Issue

AVO-234

Description

This PR adds a tooltip to the production source legend list in the BreakdownChart.

Preview

image image

Mobile:
image
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.

@silkeholmebonnen
Copy link
Contributor Author

@Alportan Do you have any feedback?

@Alportan
Copy link
Contributor

Alportan commented Jul 9, 2024

@Alportan Do you have any feedback?

Looking good! 😎 Maybe one small thing, could we use the frosted background blur on the modal? 🙌
image

I believe we already use it on a few components/elements.

@VIKTORVAV99
Copy link
Member

@Alportan Do you have any feedback?

Looking good! 😎 Maybe one small thing, could we use the frosted background blur on the modal? 🙌 image

I believe we already use it on a few components/elements.

I have been using background-blur (not sm) to not get the edges to bleed tough, it would be nice if it was the same here :)

@@ -1,5 +1,9 @@
import * as Portal from '@radix-ui/react-portal';

Check warning

Code scanning / ESLint

Disallow unused variables Warning

'Portal' is defined but never used. Allowed unused vars must match /^_/u.
@silkeholmebonnen
Copy link
Contributor Author

I added the blur and I also changed the mobile version to not be in the middle of the screen, but right above or below the production source icons, because thats what it looked like in figma. Will you take a look at it again @Alportan, and let me know if you would rather have in be in the center on mobile.

@Alportan
Copy link
Contributor

I added the blur and I also changed the mobile version to not be in the middle of the screen, but right above or below the production source icons, because thats what it looked like in figma. Will you take a look at it again @Alportan, and let me know if you would rather have in be in the center on mobile.

Looks good with the blur! 🫶

Regarding the center modal on mobile, that's a good question. I'm unsure about it for now because adding a tooltip on top of the graph might be disruptive, as the main goal is to help users identify sources on the graph.

It's important to keep the design consistent and avoid too many custom components or logic. However, we should review all current use-cases first. There’s a future task to assess and unify tooltips, so we can start with this implementation and refine it later as needed.

@silkeholmebonnen
Copy link
Contributor Author

I added the blur and I also changed the mobile version to not be in the middle of the screen, but right above or below the production source icons, because thats what it looked like in figma. Will you take a look at it again @Alportan, and let me know if you would rather have in be in the center on mobile.

Looks good with the blur! 🫶

Regarding the center modal on mobile, that's a good question. I'm unsure about it for now because adding a tooltip on top of the graph might be disruptive, as the main goal is to help users identify sources on the graph.

It's important to keep the design consistent and avoid too many custom components or logic. However, we should review all current use-cases first. There’s a future task to assess and unify tooltips, so we can start with this implementation and refine it later as needed.

Okay, I'll keep it as it is for now then:)

@silkeholmebonnen
Copy link
Contributor Author

/review

Copy link
Contributor

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Possible Bug:
The use of key={index} in the map function within ProductionSourceTooltipInner might lead to issues if the list of sources can change dynamically. It's better to use a unique identifier from the source object if available.

Code Duplication:
The styling for the tooltip in mobile and non-mobile modes is very similar and could potentially be refactored into a single component or function to reduce redundancy and improve maintainability.

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! 🎉

Just one minor thing but nothing blocking! Nice work!

Comment on lines +30 to +32
{sources.map((source, index) => (
<ProductionSourceLegend key={index} electricityType={source} />
))}
Copy link
Member

Choose a reason for hiding this comment

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

As the bot suggested you can use the source object directly as a key here as well just as you did later in the code.

@silkeholmebonnen silkeholmebonnen enabled auto-merge (squash) July 11, 2024 08:20
@silkeholmebonnen silkeholmebonnen merged commit 73cefc8 into master Jul 11, 2024
20 checks passed
@silkeholmebonnen silkeholmebonnen deleted the silkebonnen/avo-234-origin-of-electricity-add-legend-tooltip-for-sources branch July 11, 2024 08:23
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.

3 participants