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

Add date range picker to energy period selector #14337

Merged

Conversation

TillFleisch
Copy link
Contributor

@TillFleisch TillFleisch commented Nov 8, 2022

Proposed change

Replace/Add the date range picker from history/logbook to the energy dashboard. This enables custom date ranges on the energy dashboard.
Pre-defined range selection is still possible using the date range picker. The arrows still serve the same functionality as before.

Original proposal:
image

Current proposal:
See comment

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

The date range picker always opens to the right and can thus potentially leave the window.
This behaviour should be changed before this PR get's approved. I did not want to mess with that part, but ideally the date picker would always open towards the middle of the window.

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:

piitaya
piitaya previously requested changes Nov 9, 2022
Copy link
Member

@piitaya piitaya left a comment

Choose a reason for hiding this comment

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

Hello 🙂
I like the idea but I think there is some design adjustment/questions :

  • As the period text length can change, the UI jumps in desktop because the buttons are displayed after the period text.
  • There is a today button inside the picker that set the period to today and another today button outside the picker that set the period start to today. It's confusing.

Also, having text buttons, text, icon buttons is not perfect. I'm sure we can improve it
image

@TillFleisch
Copy link
Contributor Author

The time handle is now to the right of the text, thus it does not jitter anymore.
This change makes the date range picker issue appear more often.
image

@TillFleisch
Copy link
Contributor Author

There is a today button inside the picker that set the period to today and another today button outside the picker that set the period start to today. It's confusing.

I agree, but I could not think of an elegant naming convention for this problem.
In my opinion we should remove the today button entirely as it's functionality is mostly provided by the date range picker. This solution would definitely be less confusing.

@TillFleisch TillFleisch requested a review from piitaya November 14, 2022 11:34
@arosboro
Copy link

Is there any remaining task for this issue which I could assist with getting it over the finish line? @TillFleisch @piitaya Happy to provide critical feedback.

@arosboro
Copy link

There is a today button inside the picker that set the period to today and another today button outside the picker that set the period start to today. It's confusing.

I agree, but I could not think of an elegant naming convention for this problem. In my opinion we should remove the today button entirely as it's functionality is mostly provided by the date range picker. This solution would definitely be less confusing.

I agree with removing the former today button if the date picker allows a today selection and it seems redundant.

@TillFleisch
Copy link
Contributor Author

One possible name for the "today" button could be "now" or "recently". This would help separate the today button and the today date picker functionality. I don't know if this terminology will translate well into other languages.

@arosboro
Copy link

I'm thinking about it more, and your addition to the widget supports ranges. Today is only a range of hours. Maybe we should leave that to the existing functionality, and refactor the date range widget to show the following:

  • This week
  • This month
  • This quarter
  • This year

This way the user could determine how granular they want it to be, and then use comparison to get this period to last period on the chart.

The capability to choose arbitrary start and stop dates is really popular with the community. It would be the critical missing piece for users to compare usage with billing periods and dates starting on a date determined by their utility provider. For instance my billing period can be anywhere from 30-34 days and starts whenever they decide to take a reading.

@TillFleisch
Copy link
Contributor Author

Removing the "today" option from the date-range picker is likely a bad idea because it's an option in every other instance of the date range picker and will thus confuse users.

I presume the reason as to why we have the separate today button is that the previous implementation only allowed single day/time-interval changes.
Comparing this month to 6 month back is possible by either pressing the previous/next button multiple times or using the "today" functionality.

With the new functionality the old "today" button is not necessary as the user can select the current period directly by either using schortcuts (within the date-range picker) or selecting the period manually.
Therefore I suggest we remove the old "today" button and rely on the existing date-range picker functionality which users are familiar with.

@TillFleisch TillFleisch force-pushed the energy-dashboard-date-picker branch from d490857 to 980e460 Compare March 4, 2023 23:32
@TillFleisch
Copy link
Contributor Author

I rebased the changes onto the current dev branch, since the date-picker dependency has been upated.
I also removed the old today button.

@sjtrny
Copy link

sjtrny commented May 30, 2023

Is there anything blocking this? Looking forward to having this integrated.

@8ctorres
Copy link

8ctorres commented Jun 7, 2023

I think this PR just got lost because it's pretty old now and the devs probably don't even look that far back when reviewing. Is there a chance that you could open another one @TillFleisch?

@TillFleisch TillFleisch force-pushed the energy-dashboard-date-picker branch from 2d0416e to 60c5d45 Compare June 20, 2023 09:19
@TillFleisch
Copy link
Contributor Author

@piitaya, i have rebased this branch/PR on dev to keep up with related changes.
I've also noticed that the image above in the PR description is outdated. Here is an updated version.
date-picker

Which further changes are necessary to close this PR?

@bramkragten
Copy link
Member

We have an updated design for the top bar of the energy dashboard that we would like to implement, that does not include a date range picker at the moment. We can look at adding it though, the overflow menu might be a good place for that.

CleanShot 2023-06-20 at 12 38 12@2x CleanShot 2023-06-20 at 12 39 12@2x

@matthiasdebaat

@TillFleisch
Copy link
Contributor Author

TillFleisch commented Jun 20, 2023

I really like the idea of having the energy period selector in the top bar at all times.

Here is another Idea:
In my opinion, we should use a combination of the two approaches. The date-range-picker eliminates the need for the day/week/month buttons and the related 'today' button (see discussion above).
image
The title of the top-bar can show the selected date(s). The kebab-menu would contain the 'Compare with previous' option.
On mobile devices, the arrow/calendar icons can be packed closer together.
(please keep in mind that I'm no design expert. This is my opinion and I'm fine with a different solution)

@TillFleisch
Copy link
Contributor Author

TillFleisch commented Jun 27, 2023

This PR now implements a combined "always in the-top-bar" approach.
The header source code is heavily inspired/copied from the Dashboard.

Desktop page:
image

Compare data in menu:
image

Small screens:
image
image
The next/prev buttons are smaller on narrow screens. Maybe someone else can find a more elegant solution than reducing size.

Energy date selection card (now wrapped in ha-card)
image

@rrozema

This comment was marked as off-topic.

@bramkragten
Copy link
Member

bramkragten commented Jun 28, 2023

@rrozema Seriously... 7 hours before your comment the change was made...

@rrozema

This comment was marked as off-topic.

@TillFleisch
Copy link
Contributor Author

I played around with this design a little more on mobile and noticed that the narrow/small time handle is quite inconvenient:
image
The normal-sized time handle on the other hand felt 'familiar', which is to be expected.

My solution for narrow screens reduces the size of the arrow buttons. This does not feel like a 'legal' move.
Here are some solutions:

  • reduce the width requirement for the narrow time-handle

    • reduce this value this.narrow = this.offsetWidth < 450; to 350 for instance
  • revert 2225c90 and deal with multi-line dates (like this constructed example)

    • image
  • reduce padding/margin between the arrows and the calendar icon (this will likely result in a similar issue)

Is there a 'smallest phone size' Home Assistant is targeting design-wise?

Does anyone have other suggestions, recommendations or opinions on this?

@TillFleisch TillFleisch force-pushed the energy-dashboard-date-picker branch from 72a0e6a to d654c23 Compare July 18, 2023 18:00
@github-actions github-actions bot added the Cast Related to Home Assistant Cast UI label Jul 18, 2023
@TillFleisch
Copy link
Contributor Author

I think the automatically applied cast label is incorrect.
@bramkragten Can you remove it?

@tvdsluijs
Copy link

It would be great to see this go live someday. Can I help?

@bramkragten bramkragten removed the Cast Related to Home Assistant Cast UI label Aug 1, 2023
@TillFleisch TillFleisch force-pushed the energy-dashboard-date-picker branch from d9cc503 to b088a4f Compare October 10, 2023 19:57
@TillFleisch
Copy link
Contributor Author

We could also show Quarters as Q3 2023 instead of Oct 1 – Dec 31. I have no clue how this works with internationalization though... Is this something you are familiar with?

@bramkragten
Copy link
Member

We could also show Quarters as Q3 2023 instead of Oct 1 – Dec 31. I have no clue how this works with internationalization though... Is this something you are familiar with?

It is not supported by the default Intl.DateTimeFormat we use now, also, I don't think it is really needed for now? 🤷‍♂️
If we do want it in the future we would need to add a translation key for it, and handle it manually.

@Madelena
Copy link
Contributor

We could also show Quarters as Q3 2023 instead of Oct 1 – Dec 31. I have no clue how this works with internationalization though... Is this something you are familiar with?

Quarters are often vague, and depending on the country - or even individual corporations - Q1 can mean anything from Jan 1 - Mar 31, or Feb 1 to Apr 30, or Jun 15 to Sep 15, etc.

@bramkragten bramkragten merged commit 6071757 into home-assistant:dev Oct 12, 2023
@teodorch85
Copy link

I think something is wrong with the latest version 2024.2.2
(I don't know if the problem existed in previous versions too)

image

image

with Energy Period Selector Plus it works properly
image

@TillFleisch
Copy link
Contributor Author

@teodorch85, please open a separate issue for this (if there isn't already one), so that we can keep track of this issue. I too can replicate this on a (core 2024.2.2/front 20240207.1) instance. The reported values don't change, once enough days have been selected so that "months" are shown in the bar charts. Adding a day in this scenario does not change the values when it should. Note that the low-carbon circle does change.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Cast Related to Home Assistant Cast UI cla-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.