Skip to content

More-Info Dialog Logbook Bug Fix#7152

Merged
bramkragten merged 2 commits intohome-assistant:devfrom
Brahmah:patch-1
Sep 29, 2020
Merged

More-Info Dialog Logbook Bug Fix#7152
bramkragten merged 2 commits intohome-assistant:devfrom
Brahmah:patch-1

Conversation

@Brahmah
Copy link
Contributor

@Brahmah Brahmah commented Sep 28, 2020

On smaller devices, the more-info dialog is rendered fullscreen. Optimizations for such devices were required.

Proposed change

Add a new @media query adding more-info logbook support for smaller devices by replacing the CSS max-height on the "ha-logbook" class with a CSS calculation. Appreciate any feedback!

Before
Before
After
After

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

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. <-- What does this mean? I can't confidently call CSS code :)

On smaller devices, the more-info dialog is rendered fullscreen. Optimizations for such devices were required.
}
@media all and (max-width: 450px), all and (max-height: 500px) {
ha-logbook {
max-height: calc(100vh - 230px);
Copy link
Member

Choose a reason for hiding this comment

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

The 230px will only work for a timeline graph, if you have any other type of graph, or no history at all it will not work correctly.

You should work with a flex box to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bramkragten Thank you, didn't play out that scenario. Will look into this further!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bramkragten The max-height is now unset on smaller screens. I believe this may provide the optimal scrolling experience on mobile. Please let me know if you disagree and prefer the flex layout.

Thanks for taking the time!

@Brahmah Brahmah requested a review from bramkragten September 29, 2020 01:12
@bramkragten bramkragten merged commit f48a282 into home-assistant:dev Sep 29, 2020
@bramkragten bramkragten mentioned this pull request Sep 30, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2022
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