Add a for attribute to tooltip#116
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a "for" property to the Tooltip component, allowing it to target and anchor to external elements. The implementation includes logic for finding the target element, managing event listeners, and calculating fixed positioning. Feedback focuses on improving robustness: addressing a potential memory leak when attaching listeners asynchronously, ensuring reactivity when the "for" property changes dynamically, and handling window resize events to maintain correct alignment.
| this.targetElement = this | ||
|
|
||
| // Defer finding the external target to ensure it is rendered | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
Using requestAnimationFrame to defer event listener attachment can lead to a memory leak. If the component is disconnected before the callback executes, listeners will be attached to the targetElement (which may be an external element) but will never be removed because disconnectedCallback has already run. Adding a check for this.isConnected ensures listeners are only attached if the component is still active.
requestAnimationFrame(() => {
if (!this.isConnected) return| headline: { type: String }, | ||
| text: { type: String }, | ||
| open: { type: Boolean, reflect: true }, | ||
| for: { type: String }, |
There was a problem hiding this comment.
The for property logic is currently only executed once during connectedCallback. If the for attribute is changed dynamically, the tooltip will not update its target or listeners. Additionally, if for is removed, the internal tooltip element will retain the position: fixed style applied by updatePosition. Consider handling these changes reactively in the updated lifecycle method to ensure the component stays in sync with its properties.
| } | ||
| } | ||
|
|
||
| updatePosition() { |
There was a problem hiding this comment.
The tooltip position is only calculated when it is first opened. If the window is resized or the layout shifts while the tooltip is visible, it will become misaligned with the target element. Consider adding a resize event listener to the window while the tooltip is open to trigger updatePosition and keep it correctly anchored.
No description provided.