Skip to content

Add seconds to some UI elements#4765

Merged
balloob merged 1 commit into
home-assistant:devfrom
KapJI:format-seconds
Feb 6, 2020
Merged

Add seconds to some UI elements#4765
balloob merged 1 commit into
home-assistant:devfrom
KapJI:format-seconds

Conversation

@KapJI
Copy link
Copy Markdown
Member

@KapJI KapJI commented Feb 5, 2020

Breaking change

UI will look slightly differently to users but it shouldn't affect anything.

Proposed change

Modern smart home should be fast and responsive. This means it should operate seconds or milliseconds but not minutes. Add new time formats with seconds to display time more precisely in the UI.

And use these new time formats to add seconds to 4 UI elements:

Before After
Logbook logbook before logbook after
System logs system logs before system logs after
History timeline history timeline before history timeline after
History chart history chart before history chart after

This was requested by some other users as well: https://community.home-assistant.io/t/show-seconds-in-logbook/108019

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

No configuration needed.

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • 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:

@KapJI KapJI requested a review from balloob February 5, 2020 14:45
@KapJI KapJI marked this pull request as ready for review February 5, 2020 15:54
@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 5, 2020

No. Your description focuses on the logbook but your code changes it everywhere in the app. That won't be accepted.

@KapJI
Copy link
Copy Markdown
Member Author

KapJI commented Feb 5, 2020

@balloob I know that it changes it everywhere and I think there is a significant value in doing so. For example, on entity cards you will be able to see the exact time when the state changed.
Please note that I added screenshots from different places in the UI, not just a logbook.

Can you please let me know what are your suggestions here? Do this just for a logbook? Just don't show users this information which is available and can be very useful? Change how it will appear?

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 5, 2020

I agree that seconds are important, but I think that we should make sure that we know exactly all places that are impacted.

For example, I think that sensors with the timestamp device class are also impacted, and I don't think that those might not need seconds.

@KapJI KapJI changed the title Add seconds for time and datetime format Add seconds to some UI elements Feb 6, 2020
@KapJI
Copy link
Copy Markdown
Member Author

KapJI commented Feb 6, 2020

@balloob I updated PR to take much safer approach and add seconds only to few most important elements. Let me know what you think of this version.

Comment thread src/common/datetime/format_date_time_with_seconds.ts Outdated
Comment thread src/common/datetime/format_date_time_with_seconds.ts Outdated
Comment thread src/common/datetime/format_date_time_with_seconds.ts Outdated
Comment thread src/common/datetime/format_time_with_seconds.ts Outdated
Comment thread src/common/datetime/format_time_with_seconds.ts Outdated
Comment thread src/common/datetime/format_time_with_seconds.ts Outdated
@KapJI
Copy link
Copy Markdown
Member Author

KapJI commented Feb 6, 2020

  • Don't use default export.
  • Generalize options support check.
  • Fix haDateTime.

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Nice! Looks great 🐬 🎉

@balloob balloob merged commit c977f22 into home-assistant:dev Feb 6, 2020
@KapJI
Copy link
Copy Markdown
Member Author

KapJI commented Feb 6, 2020

@balloob Thank you for reviewing this!

Please let me give you some piece of feedback from the stranger. I find your initial reply here not very nice. Developers from all over the world are trying to make your project a tiny bit better and they'd probably expect feedback about their contributions like "Thanks, but there are some things you need to consider..." rather than "No. This won't be accepted.". One man wrote on his twitter profile that it's important to be nice. So please be nice and don't become Linus.

I really love your project and promote it within my friends so keep doing the great things 🙂

@lock lock Bot locked and limited conversation to collaborators Feb 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants