Skip to content

Conversation

@Maschga
Copy link
Collaborator

@Maschga Maschga commented Dec 29, 2025

Fix #26212

📝 show loadpoint in two row style in lg breakpoint range (extracted from #20499)

\cc @naltatis

@evcc-bot evcc-bot added enhancement New feature or request ux User experience/ interface labels Dec 29, 2025
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • Consider disposing the loadpointTitleTooltip instance in unmounted (and possibly when the element stops overflowing) instead of only disabling it to avoid lingering tooltip state and potential memory leaks.
  • The global resize and orientationchange listeners call updateLoadpointToolTip on every event; adding a small debounce or throttle would reduce layout thrashing from repeated scrollWidth/clientWidth measurements on rapid resize.
  • In updateLoadpointToolTip, it may be safer to assert/guard that this.$refs.loadpointTitle is an HTMLElement (and not, for example, an array of elements) before accessing layout properties, to avoid runtime errors if the template changes.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider disposing the `loadpointTitleTooltip` instance in `unmounted` (and possibly when the element stops overflowing) instead of only disabling it to avoid lingering tooltip state and potential memory leaks.
- The global `resize` and `orientationchange` listeners call `updateLoadpointToolTip` on every event; adding a small debounce or throttle would reduce layout thrashing from repeated `scrollWidth/clientWidth` measurements on rapid resize.
- In `updateLoadpointToolTip`, it may be safer to assert/guard that `this.$refs.loadpointTitle` is an `HTMLElement` (and not, for example, an array of elements) before accessing layout properties, to avoid runtime errors if the template changes.

## Individual Comments

### Comment 1
<location> `assets/js/components/Loadpoints/Loadpoint.vue:343-349` </location>
<code_context>
+		window.addEventListener("orientationchange", this.updateLoadpointToolTip);
+		this.updateLoadpointToolTip();
 	},
 	unmounted() {
 		if (this.tickerHandler) {
 			clearInterval(this.tickerHandler);
 		}
+		window.removeEventListener("resize", this.updateLoadpointToolTip);
+		window.removeEventListener("orientationchange", this.updateLoadpointToolTip);
 	},
 	methods: {
</code_context>

<issue_to_address>
**suggestion (performance):** Dispose the Bootstrap tooltip instance on unmount to avoid potential leaks and dangling event listeners.

In `unmounted`, you clean up the window listeners but keep the Bootstrap `Tooltip` instance. Since tooltips attach their own listeners and references, also call `this.loadpointTitleTooltip?.dispose()` (and optionally set `this.loadpointTitleTooltip = null`) to fully release resources.

```suggestion
	unmounted() {
		if (this.tickerHandler) {
			clearInterval(this.tickerHandler);
		}

		// Dispose Bootstrap tooltip instance to avoid leaks and dangling listeners
		this.loadpointTitleTooltip?.dispose();
		this.loadpointTitleTooltip = null;

		window.removeEventListener("resize", this.updateLoadpointToolTip);
		window.removeEventListener("orientationchange", this.updateLoadpointToolTip);
	},
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@andig andig requested a review from naltatis December 29, 2025 18:04
@naltatis naltatis self-assigned this Dec 30, 2025
@naltatis
Copy link
Member

@Maschga Lets not do the tooltip. This is a user-controlled field. Nothing unexpected should be here the would require this. We dont have a reliable (non-complex) way of only showing the tooltip if needed (css layouting). Tooltips generally work sub-optimal on mobile devices.

@Maschga
Copy link
Collaborator Author

Maschga commented Dec 31, 2025

Nothing unexpected should be here the would require this

I added the tooltip because the user has no way of reading the complete loadpoint title if it has been truncated.
But I understand your point too. I'll revert it.

@naltatis
Copy link
Member

naltatis commented Dec 31, 2025

Looks good. But I've just rediscovered a special case that we also need to handle:

/* breakpoint lg, tall screen, 2 loadpoints rows */
@media (min-width: 992px) and (min-height: 1450px) {
	.carousel--2 {
		grid-gap: 4rem;
		grid-template-columns: 1fr;
	}
}

/* breakpoint lg, taller screen, 3 loadpoints rows */
@media (min-width: 992px) and (min-height: 1900px) {
	.carousel--3 {
		grid-gap: 4rem;
		grid-template-columns: 1fr;
	}
}

We have special "tall-screen" styling. Mostly for wall mounted tablets. In these cases the preferred layout for the card header would be one-row, since space is available. If we could ignore older browser we could try container query css but these wall mounted tables often are old devices :/

Bildschirmfoto 2025-12-31 um 12 06 47

@Maschga
Copy link
Collaborator Author

Maschga commented Dec 31, 2025

If we could ignore older browser we could try container query css but these wall mounted tables often are old devices :/

I'm happy to put in a little extra effort for this, because I now have one of these things hanging on my wall myself. :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request ux User experience/ interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Loadpoint ellipsis on tablets (landscape orientation)

3 participants