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

fix(web): axis units overflowing graphcard #6754

Merged
merged 13 commits into from
May 27, 2024
Merged

Conversation

silkeholmebonnen
Copy link
Contributor

@silkeholmebonnen silkeholmebonnen commented May 15, 2024

Issue

These two pictures show 2 issues related to axis units overflowing.

This issue is caused by the formatCo2 function not handling negative values correctly, partly because the value to match set in NetExchangeChart was not taking into account that the number could be negative, and partly because the method itself didn't account for negative values.
image
The second issue is that the gray overlay was not wide enough to cover the axis units.
image

Description

This PR fixes these two issues so that the axis show negative values correctly and it widens the gray overlay.

Preview

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.

@@ -20,7 +20,7 @@ import GraphHoverLine from './GraphHoverline';
import ValueAxis from './ValueAxis';

const X_AXIS_HEIGHT = 20;
const Y_AXIS_WIDTH = 20;
const Y_AXIS_WIDTH = 26;
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea here, but not sure if it should be in this PR or just a future improvement but it would be nice to calculate the actual width needed instead of having to use a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that would be nice! In the design review meeting we discussed units which will also affect how much space is needed, so I think it is a future thing.

VIKTORVAV99
VIKTORVAV99 previously approved these changes May 15, 2024
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!

@silkeholmebonnen
Copy link
Contributor Author

silkeholmebonnen commented May 15, 2024

I just changed some things after we discussed it in the meeting. I should have let you know, sorry @VIKTORVAV99

@silkeholmebonnen
Copy link
Contributor Author

It should be ready for review again now!

@VIKTORVAV99 VIKTORVAV99 self-requested a review May 15, 2024 13:26
@madsnedergaard
Copy link
Member

madsnedergaard commented May 15, 2024

Wow, you very quickly and easily just fixed the disabled message - you're awesome! 💪

silkeholmebonnen and others added 2 commits May 27, 2024 10:35
Co-authored-by: Viktor Andersson <30777521+VIKTORVAV99@users.noreply.github.com>
@silkeholmebonnen
Copy link
Contributor Author

Ready for review again ⭐

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

@silkeholmebonnen silkeholmebonnen merged commit 189d1d1 into master May 27, 2024
21 checks passed
@silkeholmebonnen silkeholmebonnen deleted the fix-graph-overflow branch May 27, 2024 12:21
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.

None yet

4 participants