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

Detail view for energy devices graph #19068

Merged
merged 11 commits into from
Feb 20, 2024

Conversation

karwosts
Copy link
Contributor

@karwosts karwosts commented Dec 17, 2023

Proposed change

New feature for the energy devices graph:

  • toggle to flip between summary view (current) and detail view (hourly/daily similar to other graphs).
  • clickable filtering in the summary view.
  • separate card for time series device usage

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

Additional information

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:

@home-assistant home-assistant bot added cla-signed WTH Issues & PRs generated from the "Month of What the Heck?" labels Dec 17, 2023
@Misiu
Copy link
Contributor

Misiu commented Jan 3, 2024

I think that a consistent look of the disabled option would look better. Now we use strikethrough (for example in history), so the clickable filters in the summary view should have the same look. WDYT?

@karwosts
Copy link
Contributor Author

karwosts commented Jan 3, 2024

I think that a consistent look of the disabled option would look better. Now we use strikethrough (for example in history), so the clickable filters in the summary view should have the same look. WDYT?

I would have also liked strikethrough but it's not an option provided by chart.js. The label strings are rendered on their canvas by their internal code, it's not something that we can style with CSS.

It may be technically possible but I think it would involve having to compute the X,Y coordinates of the start and end of each label and draw the strikethrough line manually with chart.js line drawing commands. But the PR was getting a bit long already and decided to punt that to maybe investigate another day.

@Misiu
Copy link
Contributor

Misiu commented Jan 3, 2024

I looked at chart.js (4.4.1) samples and all of them have strikethrough when you disable an item in the legend - https://www.chartjs.org/docs/latest/samples/bar/horizontal.html
But that can be checked/changed/unified later.

@karwosts
Copy link
Contributor Author

karwosts commented Jan 3, 2024

Yes that is the legend, but what we are discussing is not the legend but the axis tick labels. The horizontal bar chart does not have a legend.

@Misiu
Copy link
Contributor

Misiu commented Jan 3, 2024

Yes that is the legend, but what we are discussing is not the legend but the axis tick labels. The horizontal bar chart does not have a legend.

D'oh.

@bramkragten
Copy link
Member

@Madelena had some ideas for this, and will come back to you

Can we split this PR in 2 PR's, one for being able to hide items in the current graph, and 1 for adding the detail view?

@karwosts
Copy link
Contributor Author

Maybe can be split, I remember a good amount of the refactoring here was related to keeping the visibility lists synchronized correctly between the two charts (so a dataset hidden in one chart was still hidden when you flipped the mode), not sure how simple that would all be to tease apart again. I'll look if I find time, otherwise maybe wait to hear the other feedback.

@karwosts
Copy link
Contributor Author

Ok I guess that was not too bad. I think I satisfied the request, just removed visibility toggles from horizontal bar chart, and forced all datasets always visible.

The detail view still has clickable visibility toggle, but that is handled just like how it is handled in all other bar charts, but it doesn't carryover to the horizontal chart.

@piitaya
Copy link
Member

piitaya commented Feb 14, 2024

Hi 👋 Thank you for the awesome feature 🤩

I discussed the PR with the product and design team and we ended with a proposal :

  • remove filtering on the horizontal bar chart graph (already done from what I see... 👍)
  • remove the button to switch between summary/graph and add separate card for the graph above summary card. In this way, we can remove the legend in the graph because the summary will be always visible below the graph.

WDYT?

@karwosts
Copy link
Contributor Author

In this way, we can remove the legend in the graph because the summary will be always visible below the graph.

I don't necessarily feel like I would want to remove the legend, as it is what allows for temporary hiding of datasets, which I feel like is a very useful feature? Is it strongly desired for that not to be interactive?

Otherwise it is very difficult if you want to focus on say behavior of a particular device, which might be too hard to view under the noise of everything else.

@piitaya
Copy link
Member

piitaya commented Feb 14, 2024

Ok let's keep the legend for now 🙂

@karwosts
Copy link
Contributor Author

Ok, I'll try pulling this out into an entirely new card and add it to the overall energy strategy.

@karwosts karwosts marked this pull request as draft February 15, 2024 01:14
@karwosts
Copy link
Contributor Author

image

@karwosts karwosts marked this pull request as ready for review February 15, 2024 18:01
@piitaya
Copy link
Member

piitaya commented Feb 16, 2024

Just tested it and it works well.
Should we have different title for each graph?

CleanShot 2024-02-16 at 16 15 31

@karwosts
Copy link
Contributor Author

Should we have different title for each graph?

Yeah I think so. Any suggestion? Couldn't decide if I should rename one or both.

@piitaya
Copy link
Member

piitaya commented Feb 16, 2024

My proposal :
Individual devices energy usage for the history chart
Individual devices for the bar chart
What do you think about this?

@ivanfmartinez
Copy link

This changes can support separated graphs for high power and low power devices ?

When looking both in same graph is very difficult to see values for the devices that use very low power compared with devices that use a lot like car, heating, pumps etc.

@karwosts
Copy link
Contributor Author

karwosts commented Feb 16, 2024

My proposal : Individual devices energy usage for the history chart Individual devices for the bar chart What do you think about this?

Hmm, can't say I'm a fan of that, given that "energy usage" seems like it could apply equally to both graphs, it doesn't really touch on what is the differentiation.

Maybe Individual devices detail usage and Individual devices summary usage? I think we can drop "Monitor" as the whole page is a monitor and is somewhat redundant. (update: went with this for now)

@karwosts
Copy link
Contributor Author

This changes can support separated graphs for high power and low power devices ?

There's only one detailed graph, but if desired you can click the legend to filter out the high power devices, and the graph will rescale to zoom into the lower power devices remaining.

@piitaya
Copy link
Member

piitaya commented Feb 16, 2024

I like your proposal @karwosts! May be we can replace summary by total as it's the total during the period?

@karwosts
Copy link
Contributor Author

I like your proposal @karwosts! May be we can replace summary by total as it's the total during the period?

That'd be fine. If you want an adjustment feel free to mark a suggested change for any of the lines in en.json and I'll accept it.

src/translations/en.json Outdated Show resolved Hide resolved
@piitaya piitaya merged commit 2a803e0 into home-assistant:dev Feb 20, 2024
13 checks passed
@piitaya piitaya added the Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Feb 20, 2024
@karwosts karwosts deleted the energy-devices-detail branch February 20, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) WTH Issues & PRs generated from the "Month of What the Heck?"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants