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 Energy-Dashboard unexpected Period Calculation #23458

Merged
merged 3 commits into from
Dec 27, 2024

Conversation

boern99
Copy link
Contributor

@boern99 boern99 commented Dec 26, 2024

Breaking change

Proposed change

In Energy-Dashboard it is possible to select exact Date Ranges. If there are more than 35 days selected then whole month is used and therefor the Energy-Distribution Calculation totally unexpected for me:
Example: Liked to know how much Grid Energy was used last 365 days and this is not what i expect:
image

This pull request can still use whole month, but if you select ranges which are not whole month, it uses Days and Energy-Distribution is on day-basis and more correct:
image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

src/data/energy.ts Outdated Show resolved Hide resolved
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft December 27, 2024 08:38
Co-authored-by: Petar Petrov <[email protected]>
@boern99 boern99 marked this pull request as ready for review December 27, 2024 09:30
@home-assistant home-assistant bot requested a review from MindFreeze December 27, 2024 09:30
Copy link
Contributor

@MindFreeze MindFreeze left a comment

Choose a reason for hiding this comment

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

Thank you

@MindFreeze MindFreeze merged commit 2ff0cb5 into home-assistant:dev Dec 27, 2024
15 checks passed
@MindFreeze
Copy link
Contributor

The caveat is that now the bar charts show too many individual days which looks bad. We should look into separating calculation and presentation

@boern99
Copy link
Contributor Author

boern99 commented Dec 27, 2024

The caveat is that now the bar charts show too many individual days which looks bad. We should look into separating calculation and presentation

Yes, you're right: separation between display and calculation would be best. I was aware that this is a quick fix. However, I noticed that the new chart zoom function could reach its full potential with smaller units, despite the currently missing comprehensive sync function for the chart periods: but that's a different topic.

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.

2 participants