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

Re-remove time picker for Energy #23891

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

karwosts
Copy link
Contributor

Proposed change

The time picker seems accidentally added to Energy during some boolean attribute refactoring. It doesn't function and shouldn't be there, I think only whole days are supported.

From: #23189

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

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:

@silamon silamon enabled auto-merge (squash) January 26, 2025 18:34
@silamon silamon disabled auto-merge January 26, 2025 18:35
@silamon
Copy link
Contributor

silamon commented Jan 26, 2025

Can't we just revert to the code it was? Having inverse code logic in the middle of passing through makes it harder to maintain.

@karwosts
Copy link
Contributor Author

I am not confident what the best way forward is. I remember a few times having similar discussions and I had the thought that having any attribute with a default value of true was discouraged, as it could not be changed via attribute.

So I don't know if this property should have the attribute removed, or what is the best fix.

@silamon
Copy link
Contributor

silamon commented Jan 26, 2025

I am not confident what the best way forward is. I remember a few times having similar discussions and I had the thought that having any attribute with a default value of true was discouraged, as it could not be changed via attribute.

So I don't know if this property should have the attribute removed, or what is the best fix.

Just had a quick look and it may be easier to remove the time picker attribute here, change the default then from true to false in the ha-date-range-picker and add the time-picker attribute to history and logbook.

@MindFreeze
Copy link
Contributor

@silamon 's suggestion sounds good. Using attributes to add things instead of removing them makes sense

@karwosts
Copy link
Contributor Author

Updated.

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.

awesome

@MindFreeze MindFreeze enabled auto-merge (squash) January 27, 2025 14:06
@MindFreeze MindFreeze merged commit d1be441 into home-assistant:dev Jan 27, 2025
11 checks passed
@karwosts karwosts deleted the energy-no-time-picker branch January 27, 2025 14:16
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.

Time range is not working in energy dashboard
3 participants