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

Fix touch control of chart zoom #23302

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Fix touch control of chart zoom #23302

merged 2 commits into from
Dec 16, 2024

Conversation

MindFreeze
Copy link
Contributor

Proposed change

Allow scrolling over a chart on touch devices when the chart is not zoomed.
Fix a bug where the chart was allowed to pan too much to the right where there is no data.

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

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

@MindFreeze MindFreeze marked this pull request as draft December 16, 2024 07:52
@MindFreeze MindFreeze marked this pull request as ready for review December 16, 2024 08:02
@@ -114,7 +114,7 @@ export class StateHistoryChartLine extends LitElement {
},
},
min: this.startTime,
suggestedMax: this.endTime,
max: this.endTime,
Copy link
Member

Choose a reason for hiding this comment

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

Why can we no longer use suggestedMax? I think there was a reason we use this instead of max 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume the reason is that we keep adding data and expanding the max, but we also pass through here every time so a hard max has the same effect.
Without hard limits the zoom plugin can break out of the limits while panning

Copy link
Member

@bramkragten bramkragten Dec 16, 2024

Choose a reason for hiding this comment

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

We have more charts with suggested max, do they all need to be changed? (Like energy charts)

I think what happend was that the point would be on very edge of the graph, causing the label to be cut off or something... Let me check if I can find it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

energy charts already use max. I got the idea from them because I couldn't reproduce the bug there while on more-info it was easy

@bramkragten bramkragten merged commit 7370d1e into dev Dec 16, 2024
15 checks passed
@bramkragten bramkragten deleted the fix-zoom branch December 16, 2024 15:03
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.

2 participants